dblaikie accepted this revision.
dblaikie added a comment.
Generally looks good, thanks!
The inline comment trying to understand the `4095 - 4088 == 7` math I think
should be answered (possibly in the form of the test/CHECK rephrasing I
mentioned to clarify what the extra 7 characters are, and maybe some more
detail in the comment too, if needed) - the rest of the suggestions are
minor/optional.
It might also be good to split up the patch and commit the new macros/cleanup
of the existing tests in one patch, then the fix+new tests in another. This way
if there's a problem with one of the patches it'd be easier to
identify/separate from the other.
================
Comment at: clang/test/CodeGenCXX/mangle-ms-md5.cpp:26-28
+int X4095(x);
+#define Y4096 X4096(y)
+#define Y4095 X4095(y)
----------------
After I wrote a few of these and realized the test needs different names (so
parameterizing the macro on the character to duplicate helps that), but then
the X became ambiguous/confusing with the literal 'x' in the names. To
avoid/reduce that confusion it might make sense to use some other letter (I had
used X for the macro then a, b, c, etc for the real identifiers - but maybe
that's still a bit confusing because "X" does tend to get used in x, y, z
example names too, even if this test file was changed to avoid that) - R for
Repeated?
Then probably the Z4089 and Z4089 could be removed (since they're only defined
on one line and used once immediately after) and the macros used directly like
on line 26 above. Though I can appreciate Y4095 being kept because it's used
several times (I could go either way on that one)
================
Comment at: clang/test/CodeGenCXX/mangle-ms-md5.cpp:27
+int X4095(x);
+#define Y4096 X4096(y)
+#define Y4095 X4095(y)
----------------
This one's unused & could be removed, I think?
================
Comment at: clang/test/CodeGenCXX/mangle-ms-md5.cpp:55
+// Verify the threshold where md5 mangling kicks in
+// Create an ident with 4088 characters, pre-hash, MangleName.size() is 4095
+#define X4088(X) \
----------------
Might be worth some more words to describe what the 7 other characters are? (to
help explain the "magic" number 4088)
Oh, one way might be to change the `CHECK-DAG` on 63 to verify the full name.
You could use a regex to match all the `z`s compactly but precisely (eg:
`@"{{\?z{4088}@@3HA}}"`)*, rather than what I guess is a sample of 4 `z`s
currently? That way it's clear what the other 7 characters are? (though I only
count 6 other characters in my small examples - so I'd expect 4088 `z` plus
those six to reach 4094, not 4095?)
* might have to muck with the regex syntax a smidge - I always forget which
things need escaping and which things don't
================
Comment at: clang/test/CodeGenCXX/mangle-ms-md5.cpp:61-62
+ C4(X256(X), X512(X), X1024(X), X2048(X)))
+#define Z4088 X4088(z) // pre-hash, MangleName.size() is 4095
+int Z4088 = 1515;
+// CHECK-DAG: {{.*}}zzzz{{.*}} = dso_local global i32 1515, align 4
----------------
As on line 26, it's probably simple enough to skip the extra macro and write
this as `int X4088(z) = 1515;`
(& as on line 26, probably skip the "= 1515" unless that adds some value (no
pun intended) to the test? I'm guessing it's not important what value, if any,
the variable is initialized with?)
================
Comment at: clang/test/CodeGenCXX/mangle-ms-md5.cpp:66-70
+#define X4089(X) \
+ C2(C2( \
+ C4(X2(X), X4(X), X4(X), X8(X)), \
+ C4(X8(X), X32(X), X64(X), X128(X))), \
+ C4(X256(X), X512(X), X1024(X), X2048(X)))
----------------
Could you align the commas and trailing backslashes here and in the X4088 one?
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D90714/new/
https://reviews.llvm.org/D90714
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
