samitolvanen added inline comments.
================
Comment at: llvm/lib/Transforms/IPO/ThinLTOBitcodeWriter.cpp:45
+ }
+ return true;
+}
----------------
nickdesaulniers wrote:
> samitolvanen wrote:
> > nickdesaulniers wrote:
> > > Can llvm::any_of or llvm::none_of be used here?
> > > llvm/ADT/STLExtras.h
> > Maybe, but I don't see how they would make this function any cleaner. Did
> > you have something specific in mind?
> Something like?
>
> return any_of(Name, [](const char &C) { return isAlnum(C) || C == '_' || C ==
> '.'; }
>
> or maybe we need !none_of(...)? (not sure if characters of a string can be
> enumerated this way)
Since we want to ensure all the characters in the string match the predicate, I
believe it would make sense to use `all_of()` instead.
However, I don't see the point of introducing additional complexity to such a
trivial function to shave off a couple of lines of code. While it might not
actually matter here, using `all_of()` also seems to generate ~5x as many
instructions to execute:
https://godbolt.org/z/Pndfxj6rM
If you feel like the current version is too long, I can drop one line by
changing the loop to use:
```
if (!isAlnum(C) && C != '_' && C != '.')
return false;
```
I initially wanted to keep the test identical to
`MCAsmInfo::isAcceptableChar()` to make it easier to see it actually matches,
but since it's no longer identical, I suppose that doesn't matter.
Thoughts?
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D104058/new/
https://reviews.llvm.org/D104058
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
