Conversation
… has been created or not. Using that helper, double check to see if the executable is in the current directory, if not then rebuild
main.cpp
Outdated
| * 7) done! | ||
| */ | ||
| try { | ||
| parse_commandline_args(argc, argv); |
There was a problem hiding this comment.
Probably should add a way for this to return a boolean or something. If we get a help message or if someone requests some option that doesn't exist we would want to exit without error instead of going through work
| const string help_header = "\ | ||
| KTPuild\n\ | ||
| An application to help making the compiling of large projects easier. The\n\ | ||
| intended use for EECS 280 and 281 students attending the University of \n\ | ||
| Michigan. Built and maintained by Kappa Theta Pi Alpha Chapter. For reporting\n\ | ||
| bugs or requesting features please visit: https://github.com/ktp-dev/KTPuild\n"; | ||
|
|
||
| const string help_msg = "\n\ | ||
| Usages:\n\ | ||
| KTPuild\n\ | ||
| KTPuild -h | --help\n\ | ||
| \n\ | ||
| Options:\n\ | ||
| -h | --help: display help information\n\n"; | ||
|
|
There was a problem hiding this comment.
Optional: use raw strings if desired to not worry about the trailing \n\ of each line.
Also, please use a consistent const char* const for global static strings unless you have to use std::string functions on the global string (and even then, I'd encourage using std::string_view). The reason is that while const string = "blah" looks innocent, it places the contents of this string into dynamic memory (the heap). This is needless bc there is already a copy of it in the static section!
| bool is_created(const string &filename){ | ||
|
|
||
| // tests for the existence of the file with success being a 0 | ||
| if(access(filename.c_str(), F_OK) == 0) | ||
| { | ||
| return true; | ||
| } | ||
| return false; | ||
| } | ||
|
|
There was a problem hiding this comment.
I think this diff will go away once you merge in pull #1 to this branch
| return false; | ||
| } | ||
|
|
||
| bool parse_commandline_args(int argc, char** &argv) |
There was a problem hiding this comment.
Is there a reason we take a char** by reference?
main.cpp
Outdated
|
|
||
| int option, optionIndex; | ||
|
|
||
| while((option = getopt_long(argc, argv, "h", longOpts, &optionIndex))!= -1) |
There was a problem hiding this comment.
What is the point of optionIndex? Does it have to be a non-null pointer, or can it be nullptr?
There was a problem hiding this comment.
It's meant to be there to cycle through all the options listened in longOpts. It is incremented every time getopt_long is called so we can just keep going till it hits the final null struct and returns back a -1
| //now we know what cpp files have changed | ||
| if (changed.empty()) //TODO: also ensure exe exists | ||
| // if there is nothing to recompile and the executable has been created we are done | ||
| if (changed.empty() && is_created(buildfile.get_executable_name())) |
main.cpp
Outdated
|
|
||
| const string help_header = "\ | ||
| KTPuild\n\ | ||
| An application to help making the compiling of large projects easier. The\n\ |
There was a problem hiding this comment.
Some english critiques that you can feel free to ignore:
"application that makes the compiling" as opposed to "help making."
"Intended for use in EECS 280"
| bugs or requesting features please visit: https://github.com/ktp-dev/KTPuild\n"; | ||
|
|
||
| const string help_msg = "\n\ | ||
| Usages:\n\ |
There was a problem hiding this comment.
Do you think we should include some more information about what the normal usage will do? Or is that better left for man pages in a later commit?
main.cpp
Outdated
|
|
||
| default: | ||
| { | ||
| return false; |
There was a problem hiding this comment.
Personally, I am a fan of using c++ exceptions to indicate error conditions. This allows the calling convention to not include error statuses and usually simplifies control logic. In this case I don't think it will have a big impact beyond encouraging future consistency. Long story short, are you opposed to throwing a std::runtime_exception here with the message equal to help_msg? This would allow the caller to do a simple if(!parse_command_line(blah)) return; // false return value means do not continue with execution and a universal handling of error cases.
Also, it avoids the subtle but potentially important bug where the return code of the program is 0 even if an invalid option was specified.
main.cpp
Outdated
| default: | ||
| { | ||
| return false; | ||
| break; |
There was a problem hiding this comment.
Also, you don't need a break after a return statement ;P
main.cpp
Outdated
| if(parse_commandline_args(argc, argv) == false) | ||
| { | ||
| cout << help_msg; | ||
| return 0; |
There was a problem hiding this comment.
See above comment about how this is buggy- will return a 0 (OK) status code even when an invalid flag was provided
|
I know I left what seems like a lot of comments, but I think you did a great job on this! Incorporating getopt_long was smart, and I liked the messages you generated! Also, please merge master into this branch to resolve those merge conflicts |
2 participants
Added ability to parse command line args as well as preliminary --help message.