Conversation
Adds support for * Path canonicalization * Making paths absolute * Add path padding character to padding signature * Stripping quotes * Backslash escaping * Corrects regex "match" bug * Adds error handling to SFN name processing * Adds file creation to sfn processing so sfns can be generated for "non existent" files * Adds method to validate a path is the proper length * Updates path padding to leverage the above api additions * Corrects a missing return statement * Adds new error types for current and future use Signed-off-by: John Parent <john.parent@kitware.com>
Signed-off-by: John Parent <john.parent@kitware.com>
src/utils.cxx
Outdated
| if (path.length() > MAX_NAME_LEN) { | ||
| // Name is too long we need to attempt to shorten | ||
| std::string const short_path = short_name(path); | ||
| // If new, shortened path is too long, bail |
There was a problem hiding this comment.
(question) I'm not clear on how this is bailing (or checking the length of the shortened path).
There was a problem hiding this comment.
The docstring should be updated. Originally I was bailing from this method if the SFN couldn't be sufficiently shortened, but now short_name handles that, and this "bails" by letting the exception propagate up.
src/utils.cxx
Outdated
| result_str = std::string(); | ||
| } else { | ||
| result_str = match.str(); | ||
| result_str = match.str(1); |
There was a problem hiding this comment.
(question) Is there a reason that e.g. regexSearch isn't also constrained to returning the first group?
There was a problem hiding this comment.
This should actually be a bit safer and check whether n > 0 where n is the number of sub matches.
I actually think I should refactor both methods (search and match) to return the match results directly and let callers decide what component to use.
To be clear overall though, regexSearch was not constrained because I was not searching with groups and always wanted the full match as search is more targeted (already matching a substring). It was bad design though.
Signed-off-by: John Parent <john.parent@kitware.com>
Signed-off-by: John Parent <john.parent@kitware.com>
Signed-off-by: John Parent <john.parent@kitware.com>
Signed-off-by: John Parent <john.parent@kitware.com>
Signed-off-by: John Parent <john.parent@kitware.com>
Signed-off-by: John Parent <john.parent@kitware.com>
Signed-off-by: John Parent <john.parent@kitware.com>
Signed-off-by: John Parent <john.parent@kitware.com>
scheibelp
left a comment
scheibelp
left a comment
There was a problem hiding this comment.
Approving (minor suggestions for later).
| // commands to overwrite it | ||
| if (!PathFileExistsA(path.c_str())) { | ||
| if (!make_file) { | ||
| char message[50]; |
There was a problem hiding this comment.
(minor) IMO it would be good to dynamically allocate based on the length of path (if the user gets a failure here, they wouldn't know which path is the issue or what it belongs to).
| * \param bsize the lengh of the padding to add | ||
| */ | ||
| char* pad_path(const char* pth, DWORD str_size, DWORD bsize) { | ||
| char* pad_path(const char* pth, DWORD str_size, char padding_char, |
There was a problem hiding this comment.
(minor) many functions return std::string here, which seems like generally the preferred approach (for memory management reasons). Was there a reason for this method specifically to return char*? I'm of the mind that it's worth explaining why whenever we return char* instead of std::string.
| // if we have single quotes, strip those | ||
| if (startswith(str, "'")) return strip(lstrip(str, "'"), "'"); | ||
| // if we have double, do that instead. | ||
| return strip(lstrip(str, "\""), "\""); |
There was a problem hiding this comment.
(minor) Curious what you think should happen for 'hello"
There was a problem hiding this comment.
Maybe I should rename this to "stripquotepairs" as IMO nothing should happen.
There was a problem hiding this comment.
In practice, it would just strip off only the leading quote, which I don't want.
2 participants
Utils: Expand api
Adds support for