nickdesaulniers added inline comments.
================
Comment at: llvm/lib/Transforms/IPO/ThinLTOBitcodeWriter.cpp:45
+ }
+ return true;
+}
----------------
samitolvanen wrote:
> 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?
> using all_of() also seems to generate ~5x as many instructions to execute:
Hard to argue with that.
Might be nice to add some test coverage of this code with an mscv target triple
though.
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
