Does a simple merge, where mergeable elements are combined, all others
are appended. Does not apply trickly namespace rules.
Details
Diff Detail
- Build Status
Buildable 8541 Build 8541: arc lint + arc unit
Event Timeline
llvm/include/llvm/Support/WindowsManifestMerger.h | ||
---|---|---|
38 | Even if C++11 allows this style, I think inserting = like this is more common. MT_String[] = { | |
70 | Conditionally defining a dtor seems tricky. It is probably better to define this regardless of the presence of libxml and do nothing if libxml is not available. | |
84 | Likewise, always define this variable and don't use it if libxml is not available. | |
llvm/lib/Support/WindowsManifestMerger.cpp | ||
29 | Prefix + Message? | |
34 | I think a better way of writing this without a global variable is this. for (StringRef S : {"application", "assembly", "assemblyIdentity", "compatibility", "noInherit", "requestedExecutionLevel", "requestedPrivileges", "security", "trustInfo"}) { if (S == ElementName) return true; } return false; |
Don't conditionally define destructor, remove global.
llvm/include/llvm/Support/WindowsManifestMerger.h | ||
---|---|---|
84 | Unfortunately this will cause the sanitizer to complain that "CombinedDoc is never used". |
llvm/include/llvm/Support/WindowsManifestMerger.h | ||
---|---|---|
61 | Why do you need this? Looks like you can just concatenate a string before passing it to the ctor. | |
llvm/lib/Support/WindowsManifestMerger.cpp | ||
34 | Format | |
99 | Do you need this reinterpret_cast? It looks like Child->name is of xmlChar * which is a typedef to unsigned char *. | |
99 | Reverse the condition so that you can continue early. | |
110–111 | else if instead of else { if ... }. | |
126 | Format |
llvm/lib/Support/WindowsManifestMerger.cpp | ||
---|---|---|
37 | I think you can remove explicit instantiation of StringRef. | |
99 | Is it done? | |
126 | There wasn't a space between for and (. Now you have it. It is probably worth reviewing your patch yourself on Phabricator after you upload a new one, as I could easily find multiple trivial errors such as indentation errors, and it is easy to find errors on Phab rather than on an editor for some reason. :) |
Early return, remove explicit StringRef
llvm/lib/Support/WindowsManifestMerger.cpp | ||
---|---|---|
99 | Oh sorry I assumed you meant the isMergeableElement should be checked first. |
llvm/lib/Support/WindowsManifestMerger.cpp | ||
---|---|---|
43 | has -> get | |
55 | Since this function returns not a boolean value but a value itself, get is a better prefix than has. | |
81–83 | The memory you allocate using strdup needs to be freed somewhere using free(). Does your patch do that? | |
110 | Remove Twine from the second expression | |
117 | I don't think you need an explicit instantiation of Twine for the second expression. | |
118 | Add {}. Usual convention is, if one statement is a block (e.g. enclosed in {})) all statements in the same if ~ else if ~ else ~ should be enclosed with {} as well. E.g. this is fine if (foo) bar; else baz; and this is also fine if (foo) { bar } else if (bar) { baz } but this isn't okay. if (foo) { bar } else baz | |
135–152 | Fix style | |
140–141 | I think we have too many occurrences of this pattern. Can you add a function that compares two xmlStrings? |
Readability improvements.
llvm/lib/Support/WindowsManifestMerger.cpp | ||
---|---|---|
81–83 | Yes, these strings become part of the node tree structure of the xml document. When xmlFreeDoc() is called by the class destructor these strings should also be freed. | |
140–141 | All the function would do is abstract away the reinterpret_cast. I think with the macro I added for cast it isn't that bad. |
llvm/lib/Support/WindowsManifestMerger.cpp | ||
---|---|---|
19–20 | In C++ it is not a very good practice to use a C macro where you can avoid it. I'd probably keep the original reinterpret_casts, but if you like this, I think I'm fine with it. | |
140–141 | What I was suggesting is adding something like this static bool streq(const unsigned char *A, const unsigned char B) { return strcmp((char *)A, (char *)B) == 0; } |
add string compare helper function.
llvm/lib/Support/WindowsManifestMerger.cpp | ||
---|---|---|
19–20 | I really prefer this, having the entire reinterpret_cast written out every time clutters the code IMO |
LGTM
llvm/test/tools/llvm-mt/simple_merge.test | ||
---|---|---|
9–40 | Please indent the expected result for the sake of readability. |
Even if C++11 allows this style, I think inserting = like this is more common.