Warning cleanup part 6

[mikelow]

This one is a bit more random as I noticed my IDE wasn't surfacing many of clang's compile-time warnings in the editor.

A couple more meaningful changes:

• removed key and vel members from SeqTrack. Made many others protected instead of public.
• replaced some constant #defines in format files with static constexprs (example: kMaxTracks).

These warnings are surfacing a couple awkward things:

1. -Wshadow warnings remind me that there's no clear variable naming convention. It's annoying that we can't have a name member and also a constructor with a name parameter without eliciting a -Wshadow warning. So do we adopt a m_ prefix for member variables? That would mean a ton of changes.
2. There are a lot of warnings concerning type narrowing, particularly around the use of size_t. It strikes me as overkill to move all VGMItem offset and length members to size_t or uint64_t, but I'm not sure what the alternative is other than keeping uint32_t and static_casting size_t to it (or changing size_t usage to uint32_t in the first place). How much do we care about supporting 4gb+ files?

Types of changes

• [ ] Bug fix (non-breaking change which fixes an issue)
• [ ] New feature (non-breaking change which adds functionality)
• [ ] Breaking change (fix or feature that would cause existing functionality to change)
• [x] Code cleanup

Checklist:

• [x] My code follows the code style of this project.
• [x] I have read the CONTRIBUTING document.

[sykhro]

This one is a bit more random as I noticed my IDE wasn't surfacing many of clang's compile-time warnings in the editor.

This makes sense. I'm preparing a PR that solves this problem and improves our handling of compiler settings

[sykhro]

Err, I don't know where the rest of my comment went.

-Wshadow warnings remind me that there's no clear variable naming convention. It's annoying that we can't have a name member and also a constructor with a name parameter without eliciting a -Wshadow warning. So do we adopt a m_ prefix for member variables? That would mean a ton of changes.

This is a job for an automated refactor using libtooling/libastmatcher. I can give it a shot if you'd like

There are a lot of warnings concerning type narrowing, particularly around the use of size_t. It strikes me as overkill to move all VGMItem offset and length members to size_t or uint64_t, but I'm not sure what the alternative is other than keeping uint32_t and static_casting size_t to it (or changing size_t usage to uint32_t in the first place). How much do we care about supporting 4gb+ files?

I find the casts to be annoying to read, I wouldn't be opposed to migrating everything to size_t for lengths and std::ptrdiff_t for offsets

[mikelow]

This is a job for an automated refactor using libtooling/libastmatcher. I can give it a shot if you'd like

That sounds great, but we should discuss style guidelines first. I'm fine with radical change and moving to a fairly standard C++ convention, snake_case and all.

I find the casts to be annoying to read, I wouldn't be opposed to migrating everything to size_t for lengths and std::ptrdiff_t for offsets

Agreed that the static_casts suck, and so I want to minimize them.

I have concerns about moving to std::ptrdiff_t / size_t. Presumably, the reason to use these types is that they best represent offset and length in an architecture-agnostic way.

My primary concern is one of practicality: since we can't make assumptions about the type-compatability of either ptrdiff_t and size_t, won't we have to litter the code with casts as we translate from byte-width-specific format code to VGMItem constructors? I understand std::ptrdiff_t is typically a signed type, so wouldn't that alone require many casts since we're typically (and I expect often unavoidably) working with unsigned types? If we were to use std::ptrdiff_t I hope there is a common using alias we could substitute, otherwise I think we'd need to invent one.

The way I look at this is that RawFiles and VGMFiles are conceptually different in a way that might influence the types we choose for their sizes and pointers. RawFiles, represent the files of whatever system the user is running on, and so it makes sense that they would utilize size_t/ptrdiff_t for size and pointer values. We want the app to support loading any file on a user's computer.

VGMFiles, on the other hand, represent the individual music files discovered within RawFiles. I think it's appropriate to assume these files will not exceed 4gb - even if the app weren't strictly dealing with old file formats this would hold true. If it simplifies our code, I think it's desirable to use a type like uint32_t to represent file size and pointers. Likewise, VGMItems are effectively subsections of VGMFile and so would use the same size/ptr type. In fact, as the code stands, VGMFile is a subclass of VGMItem and inherits dwOffset and unLength, the former of which is used to represent a VGMFile's offset within its parent RawFile. If we want VGMItem and RawFile to use different size/ptr types, VGMFile will need to decouple from VGMItem::dwOffset and use RawFile's ptr type to represent offset.

I'm happy to be corrected on any of this. Let me know your thoughts :).

[sykhro]

That sounds great, but we should discuss style guidelines first. I'm fine with radical change and moving to a fairly standard C++ convention, snake_case and all.

I suggest having a look at Serenity OS's

VGMFiles, on the other hand [...]

True, I was mostly thinking in terms of RawFile. Let's go with unit32_t then, when I made that proposal I was misremembering dwOffset as signed. Perhaps it might also be useful to have a strong-type alias to that to distinguish between relative/absolute offsets

[mikelow]

I suggest having a look at Serenity OS's

I just read through it and I really like it. Two things I'm not sure I'm sold on:

• East const over West const.
• including both virtual and override (or final) keywords.

I don't feel super strongly about either. So I'm fine to roll with it if it's your preference.

[sykhro]

I suggest having a look at Serenity OS's

I just read through it and I really like it. Two things I'm not sure I'm sold on:

• East const over West const.
• including both virtual and override (or final) keywords.

I don't feel super strongly about either. So I'm fine to roll with it if it's your preference.

Right, as much as I get East const I find it confuses potential contributors. About the second one, I guess that it's done so that it's easier to distinguish virtual functions introduced at some point of the inheritance hierarchy easier? Think C inherits B inherits A and defining a virtual function in B in addition to those in A

[mikelow]

Actually, there's one major thing I don't love about the Serenity OS style guide: function names are snake_case.

I think if I had to choose, I would go for lowerCamelCase a la the Swift guidelines, but I expect this is very non-standard for C++. Otherwise I would prefer UpperCamelCase, which seems to be more common than snake_case.

I think either of the camel cases make code more readable, as they create a visual contrast to snake_cased variables and members.

What are your thoughts?

[mikelow]

Oh, more significantly, Qt uses lowerCamelCase. There's that precedent, at least.

[mikelow]

About the second one, I guess that it's done so that it's easier to distinguish virtual functions introduced at some point of the inheritance hierarchy easier? Think C inherits B inherits A and defining a virtual function in B in addition to those in A

I would think the opposite. I'd expect that excluding virtual except on its first declaration (meaning no override) would make this distinction clearer.

Not an issue I feel too strongly on, though.

[sykhro]

I think if I had to choose, I would go for lowerCamelCase a la the Swift guidelines, but I expect this is very non-standard for C++. Otherwise I would prefer UpperCamelCase, which seems to be more common than snake_case.

I think either of the camel cases make code more readable, as they create a visual contrast to snake_cased variables and members.

This is actually pretty standard in C++. A common mix I see is:

• type names in CamelCase
• function names in lowerCamelCase
• variable names in snake_case

in the end, the STL is snake_case + CamelCase; a ton of libraries (like Qt) are CamelCase

[mikelow]

That combination sounds ideal to me. How do you feel about it?

[sykhro]

Sounds good to me, let's go with that.