Page MenuHomePhabricator

[ADT] Rename StringRef case insensitive methods for clarity
ClosedPublic

Authored by mstorsjo on Jun 23 2021, 2:08 PM.

Details

Summary

Rename functions with the xx_lower() names to xx_insensitive().
This was requested during the review of D104218.

Test names and variables in llvm/unittests/ADT/StringRefTest.cpp
that refer to "lower" are renamed to "insensitive" correspondingly.

Unused function aliases with the former method names are left
in place (without any deprecation attributes) for transition purposes.

All references within the monorepo will be changed (with essentially
mechanical changes), and then the old names will be removed in a
later commit.

Also remove the superfluous method names at the start of doxygen
comments, for the methods that are touched here. (There are more
occurrances of this left in other methods though.)

Diff Detail

Event Timeline

mstorsjo created this revision.Jun 23 2021, 2:08 PM
mstorsjo requested review of this revision.Jun 23 2021, 2:08 PM
Herald added projects: Restricted Project, Restricted Project, Restricted Project, Restricted Project, Restricted Project. · View Herald TranscriptJun 23 2021, 2:08 PM

Given how large this is, would it be reasonable to split this up a bit more?

What I might do if this were my patch: get a review of the API change + the manual changes in one patch (assuming there aren't many manual changes), then land the remaining mechanical changes in chunks, perhaps vaguely by component, likely using post-commit review. The benefit of committing by chunks is that if there is some problem that comes up (even mechanical changes fail) there's more granularity for bisection (and less churn on revert). WDYT?

Also: is there a reference to this API in the ProgrammersManual? (I honestly don't know, but if there is, please be sure to update it as well.)

MaskRay added inline comments.Jun 23 2021, 4:22 PM
llvm/include/llvm/ADT/StringRef.h
192

https://llvm.org/docs/CodingStandards.html#doxygen-use-in-documentation-comments

"Don’t duplicate function or class name at the beginning of the comment."

Given how large this is, would it be reasonable to split this up a bit more?

What I might do if this were my patch: get a review of the API change + the manual changes in one patch (assuming there aren't many manual changes), then land the remaining mechanical changes in chunks, perhaps vaguely by component, likely using post-commit review. The benefit of committing by chunks is that if there is some problem that comes up (even mechanical changes fail) there's more granularity for bisection (and less churn on revert). WDYT?

That sounds like a good plan to me, thanks for the suggestion!

Also: is there a reference to this API in the ProgrammersManual? (I honestly don't know, but if there is, please be sure to update it as well.)

I don't think so, at least grepping for _lower didn't hit anything in llvm/docs (and manually browsing it now, the section on StringRef doesn't mention those methods).

llvm/include/llvm/ADT/StringRef.h
192

Thanks, I'll fix those for the methods I'm renaming here, but I'll refrain from touching the other methods in this patch.

mstorsjo updated this revision to Diff 354178.Jun 24 2021, 2:42 AM

Reduced this patch only to updating of the StringRef class itself and its unit test, removing the mechanical changes. Removed superfluous method name duplication in doxygen comments.

mstorsjo edited the summary of this revision. (Show Details)Jun 24 2021, 2:43 AM
dexonsmith accepted this revision.Jun 24 2021, 11:54 AM

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? Just be sure to clang-format when you do the mechanical changes in the follow up patches.)

llvm/include/llvm/ADT/StringRef.h
198

You could probably drop the LLVM_NODISCARD from the soon-to-be-deleted _lower variants, but maybe it's not worth fussing with (up to you).

llvm/lib/Support/StringRef.cpp
37

You can drop these duplicate doxygen comments entirely from the .cpp file.

This revision is now accepted and ready to land.Jun 24 2021, 11:54 AM
MaskRay accepted this revision.Jun 24 2021, 12:02 PM
lattner accepted this revision.Jun 24 2021, 12:03 PM

Thank you!

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).

llvm/lib/Support/StringRef.cpp
37

Sure, yeah doxygen in implementation files make little sense anyway.

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).

Sounds good. (I'm generally in favour of erring on the side of trusting clang-format, especially for large refactorings like this; I wouldn't spend too much time analyzing whether it's an improvement...)

This revision was landed with ongoing or failed builds.Jun 24 2021, 2:23 PM
This revision was automatically updated to reflect the committed changes.