[PATCH] D38643: PR13575: Fix USR mangling for fixed-size arrays.

[Jan Korous via Phabricator via cfe-commits]

jkorous-apple added a comment.

Replied and going to delete the end delimiter.



================
Comment at: lib/Index/USRGeneration.cpp:819
     }
+    if (const ArrayType *const AT = dyn_cast<ArrayType>(T)) {
+      Out << "{";
----------------
arphaman wrote:
> You can use `const auto *` here.
That's what I wanted to do in the first place but decided against it because of consistency - see other if conditions around. Do you still advice to use auto?


================
Comment at: lib/Index/USRGeneration.cpp:826
+      }
+      if (const ConstantArrayType* const CAT = dyn_cast<ConstantArrayType>(T)) {
+        Out << CAT->getSize();
----------------
arphaman wrote:
> Ditto. Also, the braces are redundant.
Braces might be redundant yet I use them intentionally even for such cases (think the infamous goto bug). But I can definitely delete those if you insist. BTW Is there some consensus about this?


================
Comment at: lib/Index/USRGeneration.cpp:829
+      }
+      Out << "}";
+      T = AT->getElementType();
----------------
arphaman wrote:
> I don't think we need the terminating character.
I agree, going to delete that.


================
Comment at: test/Index/USR/array-type.cpp:1
+// RUN: c-index-test core -print-source-symbols -- %s | grep "function(Gen,TS)/C++" | grep foo | cut -s -d "|" -f 4 | uniq | wc -l | grep 3
+
----------------
arphaman wrote:
> Please use `FileCheck` and verify the exact USR strings. This way you'll know that they're unique without actually trying to verify if they're unique in the test.
Since the USR format doesn't seem to be really stable I wanted to avoid hard-coding USR values in tests. Wouldn't those tests be unnecessary brittle in the sense that hard-coded values would have to be modified in case of future changes?


https://reviews.llvm.org/D38643