The title of commit 2 isn't very descriptive ("restructure" how?), and in fact is misleading, since it contains functional changes as well.
The "save_str" local variable seems redundant; it's never modified.
Patches 3 and 4 have "shlwapi" as a prefix; that should probably be "kernelbase" instead.
From patch 3:
- /*
* url has to contain at least one colon.* The scheme part length has a minimum of 2 characters.* Output buffer length has to be minimum of 3 characters, scheme + colon.** Return S_FALSE if url and buffer do not meet this minimum requirements.*/- helper_str = wcschr(url, L':');
- if (!helper_str)
return S_FALSE;
"helper_str" is never used in this patch.
The comment makes reference to scheme length but doesn't validate it.
The line about S_FALSE strikes me as somewhat unnecessary; the code tells us as much.
From patch 4:
- /*
* Colon should not be at the beginning*/
No need for these comments to span three lines.
- colon_pos = helper_str-url;
- if (helper_str && (1 >= colon_pos))
return S_FALSE;
I find conditionals hard to read when phrased like this; can we please put the constant on the right side?
- /*
* Check if string fits into maxChars*/
This comment, and others like it, seems redundant; the code tells us as much.
- // TODO: Use own buffer for adjustments, output buffer may not fit before fix
We don't use C++ comments.
- if (colon_pos >= maxChars && maxChars < 3)
return S_FALSE;
Why the "maxChars < 3" part?
- /*
* Set potential scheme*/- lstrcpynW(save_str, url, pos+2);
- url = url + colon_pos + 1;
This looks wrong. Either the following loop will overwrite this, or we'll return S_FALSE, in which case we probably shouldn't be touching the output buffer.
In fact, should we even be writing the scheme if we might fail later?
- /* Return false in most remaining cases should be safe */
- return S_FALSE;
This comment seems out of place. Either it's correct or it's not.
- /*
* Concat L"://"" for ftp, http, https scheme * else ":"*/
This all seems irrelevant to the patch subject; it should be part of its own patch.
This commit also doesn't quite match the code. We already copied the colon; we're not doing it here.
- if ( 0 == lstrcmpW(save_str, L"ftp:") ||
0 == lstrcmpW(save_str, L"http:") ||0 == lstrcmpW(save_str, L"https:") )
Let's please consistently use standard wcs* functions, instead of mixing lstr* and wcs*. I would also just simply say "!wcscmp", but I guess to each their own.
- {
if ( url[0] == L'\\' || url[0] == L'/' ){url++;if ( url[0] == L'\\' || url[0] == L'/' )url++;}lstrcatW(save_str, L"//");
This is missing destination size checks. Also, if we don't have enough room, should we really be touching the output buffer earlier, viz. to write the scheme?
- }
- /*
* Remove leading "/", "\" and ":" from url.* Output already have them if needed*/- while ( url[0] == L'\' || url[0] == L'/' )
- {
lstrcatW(save_str, L"/");url++;- }
The comment doesn't match the code here, and there's no tests for this.