Closed Hacklin closed 5 years ago
@anthwlock Do you want me to fix the non-const references?
Example:
uint32_t readBits(int n, const uint8_t** buffer, int* offset);
called: readBits(2, &buffer, &offset);
See: Google C++ Style Guide: Reference Argumentsuint32_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-constOk, 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.
Ok, but use
const uint8_t* buffer
instead ofconst 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?
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.
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?
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.
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.
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.
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 ...
// 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.
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:
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.
Google C++ Style Guide See: https://google.github.io/styleguide/cppguide.html
Changes:
TODO: