search

BlackToppStudios / Mezz_Foundation

Foundational Data types that enforce no opinions on how to build games and provide many facilities that complex games need. This largely avoids graphics and physics, but provides tools like the SortedVector, CountedPtr and HashedString classes.
GNU General Public License v3.0
3 stars 0 forks source link

Added simple class and functions for handling command line args #52

Closed Sqeaky closed 5 years ago

MakoEnergy commented 5 years ago

As a side note, this functionality may be something we want to put off into another library focused on utilities for executables. But for now I'm fine with it's inclusion in the Foundation until we have more such utilities.

1) Line 63 of CommandLine.h you say "nain" instead of what I think is meant to be "main". 2) Use of "Mezzanine::String" seems needlessly verbose (as opposed to "String"). 3) I'm curious how the linking for the "ArgToken" variable will be handled. Normally I'd say slap an inline on that to be more safe/explicit but Ubuntu is giving us issues with that. 4) Line 101 of CommandLine.h, first word is "argument" and I think should say "arguments". 5) Line 124 of CommandLine.h ends with "Short arguments have one "-", long have two" and I think it would be clearer by saying "Short arguments have one ArgToken ("-"), and long arguments have two ArgTokens". 6) Line 182 of CommandLine.h has an extra curly bracket "}". 7) Rule of 5 for the CommandLine class? As a side note of this, having const members makes it not movable (if we even care). 8) Line 44 of CommandLineTests.h is missing it's "brief" text. 9) In the tests you seem to be saving the warning state, doing nothing with it, then restoring it.

That's it for my first pass. I'll give it another pass after edits and before merging.

Sqeaky commented 5 years ago

Those all look reasonable, I will look into them today.

Sqeaky commented 5 years ago
  1. Fixed
  2. Fixed
  3. I think this was changed in C++11/14 so that the whole separate definition/declaration on simple read only globals goes away. I am fairly old ODR broke this and new ODR collapses multiple inclusions of this into one definition.
  4. Fixed
  5. Fixed
  6. Fixed
  7. Fixed - I hadn't thought this one through all the way because I tried to make this a simple read only class and added one constructor to make that happen. I am deleting default construction and assignment because those are all non-sense on read only classes. I am adding default destructor and copy/move constructors. I am adding my rationale for this to the documentation of the class. The short form of the rationale, the args main got are read only, and this is a way to read those, manipulation of them is probably a mistake.
  8. Fixed
  9. Fixed
MakoEnergy commented 5 years ago

The parameters for the copy constructor and copy assignment operator should be const. Otherwise this looks good.

Sqeaky commented 5 years ago

Oh, I see. That was dumb of me. Earlier when we were talking I thought you meant one of the functions that accept the argc and argv.

I will upload a fix momentarily.