This patch extends the already existing facility to add 'const' to variables
to be more flexible and correct. The previous version did not consider pointers
as value AND pointee. For future automatic introduction for const-correctness
this shortcoming needs to be fixed.
It always allows configuration where the 'const' token is inserted, either on
the left side (if possible) or the right side.
It adds many unit-tests to the utility-function that did not exist before, as
the function was implicitly tested through clang-tidy checks. These
tests were not changed, as the API is still compatible.
Details
Diff Detail
- Repository
- rG LLVM Github Monorepo
- Build Status
Buildable 41088 Build 41253: arc lint + arc unit
Event Timeline
unittests/clang-tidy/AddConstTest.cpp | ||
---|---|---|
733 ↗ | (On Diff #173569) | It would be interesting to see test cases with multiple instantiations of the template the fix applies to. |
unittests/clang-tidy/AddConstTest.cpp | ||
---|---|---|
733 ↗ | (On Diff #173569) | I added test for a template function with many instantiations, but there should not be a difference between the instantiations, because only the original code would be transformed, and there the 'how it looks' counts, so in this case it will be treated as a value. |
Looks reasonable to me (or at least all these tests make this look good :)), but i'm not sure i can fully review this.
clang-tidy/utils/FixItHintUtils.cpp | ||
---|---|---|
35 ↗ | (On Diff #175444) | This file is replicating a bunch of functionality that exists on the Type* already. Why do this rather than have the caller do QT->isArrayType() and skip this function entirely? (Same exists elsewhere here.) |
109 ↗ | (On Diff #175444) | always right -> always to the right |
clang-tidy/utils/FixItHintUtils.h | ||
47 ↗ | (On Diff #175444) | I feel like this could be trivially generalized to any type qualifier, not just const. How would you feel about renaming this to: addQualifierToVarDecl() and adding a parameter of type DeclSpec::TQ to specify which qualifier to add? |
clang-tidy/utils/FixItHintUtils.cpp | ||
---|---|---|
35 ↗ | (On Diff #175444) | I had the issue that typedefs are resolved, but shouldn't. If I am not mistaken QT->isArrayType() would go to the canconical type. using MyPtr = int*; MyPtr foo = nullptr; Is treated wrong in that case. |
clang-tidy/utils/FixItHintUtils.h | ||
47 ↗ | (On Diff #175444) | Yes, sounds good. |
another ping. @alexfh and @aaron.ballman you commented on prior versions. Would be nice if you could take a (final) look at this patch!
A few more comments.
clang-tidy/utils/FixItHintUtils.cpp | ||
---|---|---|
83 ↗ | (On Diff #182345) | The first operand being llvm::Twine is enough for the whole chained concatenation expression to be llvm::Twine. |
unittests/clang-tidy/AddConstTest.cpp | ||
15 ↗ | (On Diff #182345) | What's the point of default values of template arguments here? |
53 ↗ | (On Diff #182345) | Is this still needed? If yes, please use "FIXME" instead and expand on what exactly is missing or needs to be done. |
unittests/clang-tidy/AddConstTest.cpp | ||
---|---|---|
15 ↗ | (On Diff #182345) | Wups, relict of older version. |
I think this looks good now, one nit about test coverage.
Did you run this through your buildbot, any issues?
clang-tidy/utils/FixItHintUtils.cpp | ||
---|---|---|
35 ↗ | (On Diff #175444) | There is a test for that, right? |
unittests/clang-tidy/AddConstTest.cpp | ||
733 ↗ | (On Diff #173569) | How about https://en.cppreference.com/w/cpp/language/function_template#Explicit_instantiation ? |
unittests/clang-tidy/AddConstTest.cpp | ||
---|---|---|
733 ↗ | (On Diff #173569) | Also, is there any test coverage missing for |
I know it has been a long time, but this patch is ready, i think :)
i would appreciate some reviews ;)
clang-tools-extra/clang-tidy/utils/FixItHintUtils.cpp | ||
---|---|---|
30–31 | ObjC pointer types? | |
34–41 | I don't see how these are improvements over checking QT->isArrayType() and friends within the caller. | |
82 | Do you need both casts for Twine? I would imagine only the first is needed. | |
166 | I think this path is reachable -- it should be an assert() instead and return None. | |
192 | I think Context should be by reference instead of by pointer. | |
228–229 | I think this is reachable as well. | |
clang-tools-extra/clang-tidy/utils/FixItHintUtils.h | ||
27 | const -> qualifier (similar for the comments in the enumerators) | |
33 | Same here. | |
37 | goes for -> attaches to? | |
45 | Comment seems out of date here as well. |
- create fresh branch with latest diff of the revision
- fix review comments
clang-tools-extra/clang-tidy/utils/FixItHintUtils.cpp | ||
---|---|---|
34–41 | True! | |
82 | In case of ' ' it is necessary, in case of " " its not. I go with " ", should not be more expensive or anything, right? | |
166 | When testing LLVM i saw some path triggering the assertions and unreachables. I want to go the None-route for now and only emit warnings without fixit. That will help to find false-positives unhandled cases better and is less problematic. Is it only objc-code that could trigger this? |
I think this generally looks good, thank you for all the hard work on this! I just found some minor nits and testing requests. Assuming no surprises with the tests, LGTM.
clang-tools-extra/clang-tidy/performance/UnnecessaryValueParamCheck.cpp | ||
---|---|---|
173–174 | Unrelated change? | |
clang-tools-extra/clang-tidy/utils/FixItHintUtils.h | ||
37 | attaches for -> attaches to | |
clang-tools-extra/unittests/clang-tidy/AddConstTest.cpp | ||
67 | I don't think this is needed -- just add a newline between the literals and let string concat work its magic? (Same elsewhere) StringRef S = "typedef int MyInt;" "MyInt target = 0;"; | |
84–85 | This does what I would expect, but boy does it make me sad to see (because target is not actually a const int * but is instead an int * const). | |
536 | Can you add a test for this scenario: struct S { int i; } x = { 0 }; where you want to apply the const to the type of x? | |
1007 | Can you also add some ObjC pointer tests? |
clang-tools-extra/unittests/clang-tidy/AddConstTest.cpp | ||
---|---|---|
67 | My Idea was to highlight the expected code-change and create both the before and after-version from the same snippets. I think with your proposal the test looks like this: StringRef S = "typedef int MyInt;" "MyInt target = 0;"; EXPECT_EQ("typedef int MyInt;" "const MyInt target = 0;", runCheckOnCode<ValueLTransform>(S)); I prefer the current version, especially with the bigger test later this file. | |
84–85 | You are right, but I am not sure what the proper policy should be. I think the smarts to detect such potential improper const should be in the library code calling the transformation. | |
1007 | i added the most basic test, do you think more is necessary? I don't know a lot about the objc languages. |
clang-tools-extra/unittests/clang-tidy/AddConstTest.cpp | ||
---|---|---|
67 | Okay, that's a good point. Also, this is testing code, so if it's a bit... odd... it's not an issue. | |
84–85 | I think the API behaves how I would expect it to and agree that the caller should be the one deciding what to add the qualifier to. It just hurts me to see the test code verifying it (but the test code is good). ;-) | |
1007 | Ah, those are regular C pointers, not ObjC pointers. I was thinking more along the lines of: @class Object; Object *g; // Try adding const to this @interface Inter - (void) foo1: (int *)arg1; // Try adding const to this parameter @end |
clang-tools-extra/unittests/clang-tidy/AddConstTest.cpp | ||
---|---|---|
1007 | Well, those don't work well. |
Still LG with a small nit in one of the FIXME tests.
clang-tools-extra/unittests/clang-tidy/AddConstTest.cpp | ||
---|---|---|
1007 |
Yes, I think you've still made good progress with the patch as-is. We can correct the ObjC stuff in a follow-up. | |
1056 | The whitespace looks incorrect here. |
clang-tools-extra/unittests/clang-tidy/AddConstTest.cpp | ||
---|---|---|
1056 | It does, but it is actually how it is expected to look. |
clang-tools-extra/unittests/clang-tidy/AddConstTest.cpp | ||
---|---|---|
1056 |
Okay, that's fair -- we usually let clang-format handle that sort of thing anyway and I cannot think of a case where the transformation would be incorrect. |
This breaks tests on Windows: http://45.33.8.238/win/5061/step_8.txt
Please take a look, and if takes a while to investigate, please revert in the meantime. (Probably needs the fno-delayed-template-instantiation kludge that's in many other tests.)
you are right. I belive it is "-fno-delayed-template-parsing" though. I added it to the default arguments runCheckOnCode for quickfix.
Unrelated change?