mstorsjo added a comment.
In D104819#2839263 <https://reviews.llvm.org/D104819#2839263>, @dexonsmith
wrote:
> LGTM, once @MaskRay is happy. I have a couple of minor comments inline too.
>
> (I also see that there are some clang-format suggestions in the unit tests;
> not sure any of them are actually improvements so maybe you left those out
> intentionally?
Yeah those seem to be intentionally vertically aligned that way, no big opinion
either way
> Just be sure to clang-format when you do the mechanical changes in the follow
> up patches.)
Hmm, those would have to be manually reviewed, but I guess I can try to do that
(discarding changes where the surrounding code isn't too well formatted now
already so it does more extensive reformattings other than for the renaming).
================
Comment at: llvm/lib/Support/StringRef.cpp:37
-/// compare_lower - Compare strings, ignoring case.
-int StringRef::compare_lower(StringRef RHS) const {
+/// compare_insensitive - Compare strings, ignoring case.
+int StringRef::compare_insensitive(StringRef RHS) const {
----------------
dexonsmith wrote:
> You can drop these duplicate doxygen comments entirely from the `.cpp` file.
Sure, yeah doxygen in implementation files make little sense anyway.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D104819/new/
https://reviews.llvm.org/D104819
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
