Maintenance: Using Google C++ Style Guide with Tabs.

[Hacklin]

Google C++ Style Guide
See: https://google.github.io/styleguide/cppguide.html

Changes:

• Use Tabs and a Tab Stop of 4 instead of 2 spaces.
• Don't indent access specifiers.

TODO:

• Fix non-const references.

[Hacklin]

@anthwlock
Do you want me to fix the non-const references?

Example:

• Google C++ Style Guide:
  declaration: uint32_t readBits(int n, const uint8_t** buffer, int* offset);
  called: readBits(2, &buffer, &offset);
  See: Google C++ Style Guide: Reference Arguments
• C++ Core Guidelines Guide:
  declaration: uint32_t readBits(int n, const uchar*& buffer, int& offset);
  called: readBits(2, buffer, offset);
  See: C++ Core Guidelines: F.17: For “in-out” parameters, pass by reference to non-const

[anthwlock]

Ok, but use const uint8_t* buffer instead of const uint8_t** buffer ;)
Please keep the typedefs uint and uchar and don't replace them. It is shorter to write uint instead of uint32_t and uchar is traditionally used for raw data.

[Hacklin]

Ok, but use const uint8_t* buffer instead of const uint8_t** buffer ;)

Ok, means Google Style I presume.
However, I cannot use const uint8_t* buffer as it doesn't advance the buffer pointer.

uint is only 4 chars shorter.
As we also need uint64, the uint becomes uint32.
And having these types just to save 2 chars of typing...

Yes, uchar is short for unsigned char and is used for raw byte data.
I'm used in dealing with octets, raw 8-bit data, so I am used to uint8_t.
To be safe, a static_assert should be added to check that uchar is the same as uint8_t or uchar could be defined as using uchar = uint8_t;.

If I read the Google C++ Style Guide on Integer Types correctly, uint8_t should be used.

Using uchar is fine with me.
I just don't like including common.h in every header file.

On the subject of integers.
Untrunc uses int, meaning int32_t, to access 32-bit video file data fields.
And I don't know if that file data is signed or unsigned.
Do you? Do we need to make it unsigned?

[anthwlock]

As long the data fields don't contain integers bigger than 2^31 it doesn't matter. Some files contain 0xFFFFFFFF indicating the file is corrupt. When using signed integer you just have to check weither the input is -1.
const uchar* buffer is a pointer to const uchar. The pointer itself is not constant. No need to make things more complex by adding a second indirection.
Why do you use preprocessor directives for commeting sections? I dont think this is a good idea.
You changed quite a lot, so don't expect this to be merged soon.

[anthwlock]

I just realised that you are right about readBits. I still dislike the double indirection though. I think the correct C++ solution is to make a ByteStream class containing bit-offsett and byte-pointer. Would you do that?

[Hacklin]

The double indirection can't be helped, when using Google C++ Style.
If you really, really dislike the double indirection,
you can make an exception to the Google Style (like you did with tab indentation)
and use C++ Core Style.

[Hacklin]

The readBits and readGolomb functions don't check for the end of the byte array from which they read. That should be fixed.

Considering the ByteStream class:
Well, that would mean re-writing the File* classes (fstream based) and the content of the Atom class (stringstream based). Note that the data is big-endian, so you have to handle the endian conversion and thus can't use the plain fstream and stringstream classes.

Instead of doing:
val32 = atom->readInt32(offset);
you would be doing:
atom->content >> seekg(offset) >> val32;.
I don't know if that's an improvement.

[anthwlock]

Perhaps StreamHelper is a better name. It should contain a uchar* and int bit-offset. So then readBits and readGolomb can take a StreamHelper object. This would also solve the double indirection, since StreamHelper takes care of it. You dont have to change the file classes for this.

Btw thanks for you contribution(s) to this branch, I will look at it in the next days.

[anthwlock]

OK. As you said yourself your changes dont affect functionality but only optic. But you have taken it to the extreme. I honor your engagement but to me it seems that only 50% of what you added/changed are worth merging. For example ...

• why do you add the GPL on top of every file? One "LICENSE" file is enough.
• usage of the preprocessor makes the code less readable.
• keep your vim settings local
• why put a full stop after every comment?
• not everything has to be aligned by whitespaces. It is more work to edit the line later.
• many things are just simply unnecessary, for example:

// Atom.
class Atom {
...

That said, I would like to merge the somewhat usefull stuff. For example...

• Type * => Type*
• typedefs {...} struct => struct {...}
• for (expr){ => for (expr) {

Again, thank you for your commitment, but this is too much.

[Hacklin]

why do you add the GPL on top of every file? One "LICENSE" file is enough.

Because Ponchio did this in the original, upstream Untrunc.
Except for AP_AtomDefinitions.h you could reduce it to something like:

// Copyright 2010 Federico Ponchio with contributions from others.
// License: GPL-2.0-or-later

Notes:

• Google Style: Legal Notice and Author Line are required on top of every file.
• GPL-2.0-or-later is defined by spdx.org.

usage of the preprocessor makes the code less readable.

Sometimes the CPreProcessor its unavoidable, like #ifndef NDEBUG or #ifdef AV_LOG_PRINT_LEVEL, or optional features.
Using #if 0 instead of // makes it clear that it is optional code and not an comment with a code example.
Using '#if 0to temporarily disable multi-line optinal code (and enabling it with#if 1[vim: Ctrl-A, Ctrl-X]) is a lot easier than adding and removing//in front of the code. Unless you always place the//at the start of the line, which is just plain ugly. (Also, Google cpplint will complain if you don't put a space after the// .) However, I also used #if 0to leave in old behavior: I'll remove those. However, I also used#if 0` for debug code:

#if 0
    // ... some debug stuff.
#endif

I'll rewrite those with:

    if (kEnableDebugStuff1) {
        // ... some debug stuff.
    }

keep your vim settings local

Yeah, this is just a convenience hack. I intend to remove them when I'm done.

why put a full stop after every comment?

Style. Comments should be more like sentences. The period indicates the end of the comment.
Also, I'm used to this and Google does it in the examples in their Style Guide.

not everything has to be aligned by whitespaces. It is more work to edit the line later.

I prefer it. It makes the source more readable.

many things are just simply unnecessary, for example:
// Atom.
class Atom {
...

As a comment, it's unnecessary.
However, I use it in the implementation file to show where the class implementation starts.
I'll change it.

That said, I would like to merge the somewhat usefull stuff.

Basically, the first patch with Google Style.