Page MenuHomePhabricator

Added llvm-string-referencing check
Needs ReviewPublic

Authored by bogser01 on Fri, Sep 25, 9:09 AM.

Details

Summary

Clang-tidy pass detecting the use of const std::string& references.

Use of llvm::StringRef is recommended in the LLVM Programmer's Manual instead:
https://llvm.org/docs/ProgrammersManual.html#the-stringref-class

Diff Detail

Event Timeline

bogser01 created this revision.Fri, Sep 25, 9:09 AM
Herald added a project: Restricted Project. · View Herald TranscriptFri, Sep 25, 9:09 AM
bogser01 requested review of this revision.Fri, Sep 25, 9:09 AM
bogser01 updated this revision to Diff 295553.Thu, Oct 1, 6:37 AM

Fixed failing unit test

bogser01 updated this revision to Diff 295580.Thu, Oct 1, 8:09 AM

Fixed unit test 2

bogser01 updated this revision to Diff 295810.Fri, Oct 2, 6:08 AM

Removed residual conflict markers

bogser01 updated this revision to Diff 295811.Fri, Oct 2, 6:11 AM

Remove conflict markers 2

@alexfh does this look alright?

Thank you for working on this check! Have you run the check over LLVM to see how much it reports? One of the concerns I have with this is that it's not always appropriate to replace a const std::string& with an llvm::StringRef and so I'm wondering what the "false positive" rate is for the check. For instance, there are std::string member functions that don't exist on StringRef so the fix-it will introduce compile errors, or switching the parameter type will cause additional unnecessary conversions.

clang-tools-extra/clang-tidy/llvm/StringReferencingCheck.cpp
24

Comments should be grammatical with capital letters and punctuation (here and elsewhere in the patch).

Also, I don't think we should register this check for the handful of C files.

26

Do we want to ignore parameters of a virtual function under the assumption that changing the parameter type may be dangerous?

27

Instead of checking isInStdNamespace(), you can use hasName("::std::string").

36

You can elide the identifiers SM and ModuleExpanderPP to shorten the declaration (since the names aren't used anyway).

51

clang-tidy diagnostics are not supposed to be grammatical, so it should not start with a capital letter and shouldn't use terminating punctuation, etc. How about something like: parameter of type 'const std::string &' could potentially be replaced with 'llvm::StringRef'?

53

I don't think the note helps a whole lot, you could attach the fix-it to the warning itself. However, we usually only add a fix-it when we can be sure that the change will continue to compile and I don't know that's the case here.

clang-tools-extra/clang-tidy/llvm/StringReferencingCheck.h
27

I think this can be hidden in the implementation file rather than exposed in the header. Also, can you pick a more descriptive name, like ParamID or something?

clang-tools-extra/docs/clang-tidy/checks/llvm-string-referencing.rst
7

Can you add some documentation?

bogser01 updated this revision to Diff 297246.Fri, Oct 9, 8:54 AM

Added documentation & Nit fixes

It looks like you generated a diff from a previous diff instead of trunk -- can you regenerate the diff against trunk so that reviewers can see the full content of the changes?

@aaron.ballman Thank you for picking up this review! Running the check over the entire LLVM causes ~74K warnings across 430 files. As to the false positive rate it's tricky to measure. Based on previous analysis on flang codebase, I would say roughly 50% of the fixes would cause compiler errors when applied.

bogser01 updated this revision to Diff 297355.Fri, Oct 9, 3:55 PM

Rebase diff

@aaron.ballman Thank you for picking up this review! Running the check over the entire LLVM causes ~74K warnings across 430 files. As to the false positive rate it's tricky to measure. Based on previous analysis on flang codebase, I would say roughly 50% of the fixes would cause compiler errors when applied.

Woof, that's a pretty high false-positive rate for the check -- I don't think we should issue a fix-it for this case because anyone who accidentally applies fixits automatically will be in for a fair amount of pain.

I'm a bit worried that the number of diagnostics this produces will basically mean the check has to be turned off for the only project that would use it. What is the expected use case for the check? For instance, are you expecting to craft a .clang-tidy file to disable this check on source files in the repo that are known to have a lot of diagnostics (so that the check mostly only fires for known-good files and new files)?

clang-tools-extra/clang-tidy/add_new_check.py
139

Looks like there are some accidental merge markers in the patch.

clang-tools-extra/docs/clang-tidy/Contributing.rst
204

More merge conflict markers.

clang-tools-extra/test/clang-tidy/checkers/llvm-string-referencing.cpp
25

You can drop the name of the check from the CHECK-MESSAGES line (we usually only check the full diagnostic once).