[PATCH] D138026: [ARM][AArch64] Use StringRef in TargetParser structs

[Tomas Matheson via Phabricator via llvm-commits]

tmatheson added a comment.

In D138026#3933447 <https://reviews.llvm.org/D138026#3933447>, @DavidSpickett wrote:

>> This is now reverse, so that the "+arch-feature" is now visible in the .def, which is a bit clearer.
>
> Clearer for what exactly.

Previously the string literal you would see in the ArmTargetParser.def was 'xyz', which would be stored on the struct as '+xyz' (a string literal created by a macro) and on the command line you would write `+xyz`. Now that it is written `+xyz` in the .def file, the correspondence between the three is more obvious, imho. Searching for `+xyz` will actually lead you to where it is defined.



================
Comment at: llvm/lib/Support/AArch64TargetParser.cpp:110
   for (const auto &AE : AArch64ARCHExtNames)
-    if (AE.Feature && ArchExt == AE.getName())
-      return StringRef(AE.Feature);
+    if (!AE.Feature.empty() && ArchExt == AE.Name)
+      return AE.Feature;
----------------
DavidSpickett wrote:
> Maybe this is just me but "!empty()" could just be "size()".
> 
> I know sometimes containers make empty cheaper but here it's just `== 0` vs `!= 0`.
For checking that "the feature is not the empty string", `!empty()` makes a lot more sense to me semantically than an implicit `cast<bool>(size())`. I'm trying to move away from the C-style code :) I would hope (haven't checked) that they compile to basically the same thing.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D138026/new/

https://reviews.llvm.org/D138026