Page MenuHomePhabricator

[clang-tidy] implement utility-function to add 'const' to variables
Needs ReviewPublic

Authored by JonasToth on Nov 11 2018, 12:15 PM.

Details

Summary

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.

Event Timeline

JonasToth created this revision.Nov 11 2018, 12:15 PM
alexfh added inline comments.Nov 12 2018, 2:00 AM
unittests/clang-tidy/AddConstTest.cpp
734

It would be interesting to see test cases with multiple instantiations of the template the fix applies to.

JonasToth updated this revision to Diff 174828.Nov 20 2018, 1:16 PM

add tests for different template instantiations

JonasToth added inline comments.Nov 20 2018, 1:18 PM
unittests/clang-tidy/AddConstTest.cpp
734

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.
Did I misinterpret your question?

JonasToth updated this revision to Diff 175444.Nov 27 2018, 3:43 AM

reabse to master + ping :)

Looks reasonable to me (or at least all these tests make this look good :)), but i'm not sure i can fully review this.

Looks reasonable to me (or at least all these tests make this look good :)), but i'm not sure i can fully review this.

No problem ;)

aaron.ballman added inline comments.Nov 29 2018, 5:56 AM
clang-tidy/utils/FixItHintUtils.cpp
34

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

108

always right -> always to the right

clang-tidy/utils/FixItHintUtils.h
47

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?

JonasToth marked 2 inline comments as done.Dec 5 2018, 5:12 AM
JonasToth added inline comments.
clang-tidy/utils/FixItHintUtils.cpp
34

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

Yes, sounds good.

  • generalize to DeclSpec::TQ
  • add dependent type-tests with typename
JonasToth marked 4 inline comments as done.Jan 17 2019, 10:59 AM
JonasToth marked an inline comment as done.
Herald added a project: Restricted Project. · View Herald TranscriptFeb 7 2019, 7:33 AM

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
82

The first operand being llvm::Twine is enough for the whole chained concatenation expression to be llvm::Twine.

unittests/clang-tidy/AddConstTest.cpp
16

What's the point of default values of template arguments here?

54

Is this still needed? If yes, please use "FIXME" instead and expand on what exactly is missing or needs to be done.

JonasToth updated this revision to Diff 188716.Feb 28 2019, 5:22 AM
JonasToth marked 3 inline comments as done.
  • address review comments
  • rebase to master
JonasToth marked an inline comment as done.Feb 28 2019, 5:22 AM
JonasToth added inline comments.
unittests/clang-tidy/AddConstTest.cpp
16

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
34

There is a test for that, right?

unittests/clang-tidy/AddConstTest.cpp
734
lebedev.ri added inline comments.Mar 17 2019, 12:28 AM
unittests/clang-tidy/AddConstTest.cpp
734