This revision was automatically updated to reflect the committed changes.
Closed by commit rL345381: [clang-tidy] Re-commit: Add new
readability-uppercase-literal-suffix check… (authored by lebedevri,
committed by ).
Changed prior to commit:
lebedev.ri added a comment.
Rerun on clang-tools-extra+lld+clang codebase, nothing broken.
I'm gonna reland.
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D52670
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
lebedev.ri updated this revision to Diff 171279.
lebedev.ri added a comment.
This revision is now accepted and ready to land.
Finally finished creducing the issue i wasn't able to understand even after
https://reviews.llvm.org/D53719+https://reviews.llvm.org/D53723,
and it turned out to be
lebedev.ri updated this revision to Diff 171172.
lebedev.ri added a comment.
Reverted in https://reviews.llvm.org/rL345305 due to multiple
apparently-lurking bugs.
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D52670
Files:
clang-tidy/cert/CERTTidyModule.cpp
This revision was automatically updated to reflect the committed changes.
Closed by commit rL344755: [clang-tidy] Add new
readability-uppercase-literal-suffix check (CERT DCL16-C… (authored
by lebedevri, committed by ).
Changed prior to commit:
aaron.ballman accepted this revision.
aaron.ballman added a comment.
LGTM! Thanks for the changes!
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D52670
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
lebedev.ri updated this revision to Diff 170108.
lebedev.ri marked 2 inline comments as done.
lebedev.ri added a comment.
CERT: also handle "lu", "llu".
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D52670
Files:
clang-tidy/cert/CERTTidyModule.cpp
lebedev.ri added a comment.
In https://reviews.llvm.org/D52670#1268572, @aaron.ballman wrote:
> In https://reviews.llvm.org/D52670#1268569, @lebedev.ri wrote:
>
> > In https://reviews.llvm.org/D52670#1268564, @aaron.ballman wrote:
> >
> > > I talked to someone at CERT responsible for maintaining
aaron.ballman added a comment.
In https://reviews.llvm.org/D52670#1268569, @lebedev.ri wrote:
> In https://reviews.llvm.org/D52670#1268564, @aaron.ballman wrote:
>
> > I talked to someone at CERT responsible for maintaining DCL16-C to get
> > their opinion on tightening the wording of the rule
lebedev.ri added a comment.
In https://reviews.llvm.org/D52670#1268564, @aaron.ballman wrote:
> In https://reviews.llvm.org/D52670#1268372, @lebedev.ri wrote:
>
> > In https://reviews.llvm.org/D52670#1268347, @aaron.ballman wrote:
> >
> > > In https://reviews.llvm.org/D52670#1268170, @lebedev.ri
aaron.ballman added a comment.
In https://reviews.llvm.org/D52670#1268372, @lebedev.ri wrote:
> In https://reviews.llvm.org/D52670#1268347, @aaron.ballman wrote:
>
> > In https://reviews.llvm.org/D52670#1268170, @lebedev.ri wrote:
> >
> > > - Apply minor wording nits.
> > > - For `cert-dcl16-c`,
lebedev.ri added a comment.
In https://reviews.llvm.org/D52670#1268347, @aaron.ballman wrote:
> In https://reviews.llvm.org/D52670#1268170, @lebedev.ri wrote:
>
> > - Apply minor wording nits.
> > - For `cert-dcl16-c`, **only** consider `L`, `LL` suffixes, not
> > **anything** else (not even
aaron.ballman added a comment.
In https://reviews.llvm.org/D52670#1268170, @lebedev.ri wrote:
> - Apply minor wording nits.
> - For `cert-dcl16-c`, **only** consider `L`, `LL` suffixes, not **anything**
> else (not even `llu`).
I'll find out about the DCL16-C recommendation, as I suspect the
lebedev.ri updated this revision to Diff 170045.
lebedev.ri marked 6 inline comments as done.
lebedev.ri added a comment.
- Apply minor wording nits.
- For `cert-dcl16-c`, **only** consider `L`, `LL` suffixes, not **anything**
else (not even `llu`).
Repository:
rCTE Clang Tools Extra
aaron.ballman accepted this revision.
aaron.ballman added a comment.
This revision is now accepted and ready to land.
LGTM aside from minor wording nits.
Comment at: clang-tidy/readability/UppercaseLiteralSuffixCheck.cpp:31-32
+ static constexpr llvm::StringLiteral SkipFirst
lebedev.ri updated this revision to Diff 169556.
lebedev.ri marked an inline comment as done.
lebedev.ri added a comment.
Bloat with `llvm::find_if()`
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D52670
Files:
clang-tidy/cert/CERTTidyModule.cpp
JonasToth added inline comments.
Comment at: clang-tidy/readability/UppercaseLiteralSuffixCheck.cpp:94
+ // Else, find matching suffix, case-*insensitive*ly.
+ for (const auto : NewSuffixes) {
+if (!OldSuffix.equals_lower(PotentialNewSuffix))
Is this
lebedev.ri updated this revision to Diff 169554.
lebedev.ri marked 8 inline comments as done.
lebedev.ri added a comment.
Hopefully address review notes:
- Support more (all?) obscure suffixes (complex, q, h, f16, i{8,16,32,64})
- Improve test coverage. There isn't coverage for every single
aaron.ballman added a comment.
Two main points: I don't think this check is covering all of the suffixes (I
don't see `q` or `i32` support, for instance), and at least for the CERT rule
this is covering, it diagnoses more than it should. The CERT rule is specific
to `l` vs `L` but imposes no
lebedev.ri added inline comments.
Comment at: clang-tidy/readability/IdentifierNamingCheck.cpp:660-678
- // Check if the range is entirely contained within a macro argument.
- SourceLocation MacroArgExpansionStartForRangeBegin;
- SourceLocation
lebedev.ri updated this revision to Diff 167661.
lebedev.ri marked 5 inline comments as done.
lebedev.ri added a comment.
Address review notes.
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D52670
Files:
clang-tidy/cert/CERTTidyModule.cpp
clang-tidy/cert/CMakeLists.txt
JonasToth added inline comments.
Comment at: clang-tidy/readability/UppercaseLiteralSuffixCheck.cpp:165
+
+ diag(LiteralLocation, "%0 literal suffix '%1' is not upper-case")
+ << LiteralType::Name << S.OldSuffix
lebedev.ri wrote:
> lebedev.ri wrote:
> >
lebedev.ri updated this revision to Diff 167658.
lebedev.ri marked 16 inline comments as done.
lebedev.ri added a comment.
Herald added subscribers: Sanitizers, llvm-commits.
Addressed remaining review notes:
- Fixed dangling links in docs
- Don't mishandle user-defined literals
- Don't ignore
lebedev.ri added inline comments.
Comment at: clang-tidy/readability/UppercaseLiteralSuffixCheck.cpp:53
+ const auto *T = dyn_cast(Node.getType().getTypePtr());
+ if (!T)
+return false;
JonasToth wrote:
> lebedev.ri wrote:
> > JonasToth wrote:
> > > Maybe
lebedev.ri marked 8 inline comments as done.
lebedev.ri added inline comments.
Comment at: clang-tidy/readability/UppercaseLiteralSuffixCheck.cpp:165
+
+ diag(LiteralLocation, "%0 literal suffix '%1' is not upper-case")
+ << LiteralType::Name << S.OldSuffix
JonasToth added a comment.
> All those are UserDefinedLiteral, so we should be good..
> https://godbolt.org/z/PcGi0B
> Also, it seems the suffix can't be set for these constants:
> https://godbolt.org/z/YHTqke
> So i'm not sure what to test. Can you give an example of a test?
I am not
lebedev.ri updated this revision to Diff 167620.
lebedev.ri marked 13 inline comments as done.
lebedev.ri added a comment.
Thank you for taking a look!
- Rebased
- Added an alias in `hicpp` module
- Addressed //most// of the review comments.
In https://reviews.llvm.org/D52670#1249898,
lebedev.ri added inline comments.
Comment at: clang-tidy/readability/UppercaseLiteralSuffixCheck.cpp:33
+};
+constexpr llvm::StringLiteral IntegerLiteral::Name;
+constexpr llvm::StringLiteral IntegerLiteral::SkipFirst;
JonasToth wrote:
> why are these
lebedev.ri added inline comments.
Comment at: docs/ReleaseNotes.rst:96
+- New alias :doc:`cert-dcl16-c
+ ` to
:doc:`readability-uppercase-literal-suffix
JonasToth wrote:
> lebedev.ri wrote:
> > Eugene.Zelenko wrote:
> > > Please move alias after new checks.
JonasToth added a comment.
Thanks for working on this!
Related as well:
http://www.codingstandard.com/rule/4-2-1-ensure-that-the-u-suffix-is-applied-to-a-literal-used-in-a-context-requiring-an-unsigned-integral-expression/
I think its wort a alias is hicpp as well
Please add tests that use
lebedev.ri updated this revision to Diff 167552.
lebedev.ri added a comment.
- Deduplicate one test.
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D52670
Files:
clang-tidy/cert/CERTTidyModule.cpp
clang-tidy/cert/CMakeLists.txt
clang-tidy/readability/CMakeLists.txt
lebedev.ri added inline comments.
Comment at: docs/ReleaseNotes.rst:96
+- New alias :doc:`cert-dcl16-c
+ ` to
:doc:`readability-uppercase-literal-suffix
Eugene.Zelenko wrote:
> Please move alias after new checks.
BTW, is there some tool to actually add this
lebedev.ri updated this revision to Diff 167547.
lebedev.ri marked an inline comment as done.
lebedev.ri added a comment.
Move alias to after the new check,
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D52670
Files:
clang-tidy/cert/CERTTidyModule.cpp
Eugene.Zelenko added a comment.
May be we should also create MISRA module?
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D52670
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
Eugene.Zelenko added inline comments.
Comment at: docs/ReleaseNotes.rst:96
+- New alias :doc:`cert-dcl16-c
+ ` to
:doc:`readability-uppercase-literal-suffix
Please move alias after new checks.
Repository:
rCTE Clang Tools Extra
lebedev.ri created this revision.
lebedev.ri added reviewers: JonasToth, aaron.ballman, alexfh, hokein, xazax.hun.
lebedev.ri added a project: clang-tools-extra.
Herald added subscribers: rnkovacs, mgorny.
Detects when the integral literal or floating point (decimal or hexadecimal)
literal has
36 matches
Mail list logo
