This is an archive of the discontinued LLVM Phabricator instance.

llvm-mt: implement simple merging of manifests, not factoring namespaces.
ClosedPublic

Authored by ecbeckmann on Jul 21 2017, 6:01 PM.

Details

Summary

Does a simple merge, where mergeable elements are combined, all others
are appended. Does not apply trickly namespace rules.

Event Timeline

ecbeckmann created this revision.Jul 21 2017, 6:01 PM

added additional test.

ruiu added inline comments.Jul 24 2017, 3:06 PM
llvm/include/llvm/Support/WindowsManifestMerger.h
38

Even if C++11 allows this style, I think inserting = like this is more common.

MT_String[] = {
74

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
73

Prefix + Message?

104

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;
ecbeckmann marked 4 inline comments as done.

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".

ruiu added inline comments.Jul 24 2017, 4:28 PM
llvm/include/llvm/Support/WindowsManifestMerger.h
65

Why do you need this? Looks like you can just concatenate a string before passing it to the ctor.

llvm/lib/Support/WindowsManifestMerger.cpp
104

Format

169

Do you need this reinterpret_cast? It looks like Child->name is of xmlChar * which is a typedef to unsigned char *.

169

Reverse the condition so that you can continue early.

180–181

else if instead of else { if ... }.

196

Format

ecbeckmann marked 4 inline comments as done.

Take away extraneous error constructor. Some format changes.

ecbeckmann added inline comments.Jul 24 2017, 5:06 PM
llvm/lib/Support/WindowsManifestMerger.cpp
169

unfortunately const char * and const unsigned char * are technically not related types, and must be casted back and forth to each other.

196

where is format error here?

ruiu added inline comments.Jul 25 2017, 12:37 PM
llvm/lib/Support/WindowsManifestMerger.cpp
107

I think you can remove explicit instantiation of StringRef.

169

Is it done?

196

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. :)

ecbeckmann marked an inline comment as done.

Early return, remove explicit StringRef

llvm/lib/Support/WindowsManifestMerger.cpp
169

Oh sorry I assumed you meant the isMergeableElement should be checked first.

Remove logging.

ruiu added inline comments.Jul 25 2017, 1:02 PM
llvm/lib/Support/WindowsManifestMerger.cpp
113

has -> get

113–117

Fix style

118–119

I think we have too many occurrences of this pattern. Can you add a function that compares two xmlStrings?

125

Since this function returns not a boolean value but a value itself, get is a better prefix than has.

151–153

The memory you allocate using strdup needs to be freed somewhere using free(). Does your patch do that?

180

Remove Twine from the second expression

187

I don't think you need an explicit instantiation of Twine for the second expression.

188

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

Readability improvements.

ecbeckmann marked 6 inline comments as done.

Readability improvements.

llvm/lib/Support/WindowsManifestMerger.cpp
118–119

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.

151–153

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.

Strip all comment and text nodes, not just in elements being merged.

ruiu added inline comments.Jul 25 2017, 2:34 PM
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.

118–119

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

ruiu accepted this revision.Jul 25 2017, 3:46 PM

LGTM

llvm/test/tools/llvm-mt/simple_merge.test
9–40

Please indent the expected result for the sake of readability.

This revision is now accepted and ready to land.Jul 25 2017, 3:46 PM
ecbeckmann marked an inline comment as done.

Indent in test.

This revision was automatically updated to reflect the committed changes.