JonasToth updated this revision to Diff 158284.
JonasToth added a comment.
Herald added subscribers: kbarton, mgorny, nemanjai.
rebase to master
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D48714
Files:
clang-tidy/cppcoreguidelines/CMakeLists.txt
clang-tidy/cppcoreguideli
JonasToth updated this revision to Diff 158285.
JonasToth added a comment.
- Merge branch 'master' into check_const
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D45444
Files:
clang-tidy/cppcoreguidelines/CMakeLists.txt
clang-tidy/cppcoreguidelines/ConstCheck.cpp
clang-ti
JonasToth updated this revision to Diff 158286.
JonasToth added a comment.
correct rebase.
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D48714
Files:
clang-tidy/hicpp/ExceptionBaseclassCheck.cpp
test/clang-tidy/hicpp-exception-baseclass.cpp
Index: test/clang-tidy/hicpp-ex
JonasToth added a comment.
I think `clang/lib/Analysis` is a good place.
> I think it should in theory be possible, though I need some advice about
> where exactly is the best place in clang.
>
> Repository:
>
> rCTE Clang Tools Extra
>
> https://reviews.llvm.org/D45679
Repository:
rCTE
JonasToth updated this revision to Diff 159062.
JonasToth added a comment.
get check up to 8.0
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D45444
Files:
clang-tidy/cppcoreguidelines/CMakeLists.txt
clang-tidy/cppcoreguidelines/ConstCheck.cpp
clang-tidy/cppcoreguidelines/
JonasToth updated this revision to Diff 159064.
JonasToth added a comment.
- get check up to date
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D40854
Files:
clang-tidy/cppcoreguidelines/CMakeLists.txt
clang-tidy/cppcoreguidelines/CppCoreGuidelinesTidyModule.cpp
clang-tid
JonasToth added inline comments.
Comment at: test/clang-tidy/hicpp-exception-baseclass.cpp:191
+void templated_thrower() { throw T{}(); }
+// CHECK-MESSAGES: [[@LINE-1]]:34: warning: throwing an exception whose type
'int' is not derived from 'std::exception'
+
J
JonasToth added a comment.
@lebedev.ri and @alexfh i would change the tests in
https://reviews.llvm.org/D48714 to use CHECK-NOTES. Is it ok, to commit this
one?
For testing purposes, you could change a single line of
`hicpp-exception-baseclass.cpp` to use the CHECK-NOTES. I do the rest :)
Re
JonasToth updated this revision to Diff 159099.
JonasToth added a comment.
- add better diagnostics about template instantiated exception types
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D48714
Files:
clang-tidy/hicpp/ExceptionBaseclassCheck.cpp
test/clang-tidy/hicpp-exc
JonasToth created this revision.
JonasToth added reviewers: arphaman, klimek.
Herald added a subscriber: cfe-commits.
This patch makes the clang-rename.py script useable for vim with only python3
support. It uses the print-function and adjust the doc slightly to mention
the correct python3 command
JonasToth updated this revision to Diff 159209.
JonasToth added a comment.
- remove double blank line
Repository:
rC Clang
https://reviews.llvm.org/D50307
Files:
tools/clang-rename/clang-rename.py
Index: tools/clang-rename/clang-rename.py
=
JonasToth added a comment.
> I'm sorry for the delay in reviewing this; I'm not certain how it fell off my
> radar for so long!
No problem :)
Comment at: clang-tidy/cppcoreguidelines/ConstCheck.cpp:32
+ *
+ * Handle = either a pointer or reference
+ * Value = everything else
JonasToth updated this revision to Diff 159220.
JonasToth marked 11 inline comments as done.
JonasToth added a comment.
- Merge branch 'master' into check_const
- [Misc] rename and first review comments
- language stuff
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D45444
Files
JonasToth added inline comments.
Comment at: clang-tidy/cppcoreguidelines/ConstCheck.cpp:141-142
+void ConstCheck::registerMatchers(MatchFinder *Finder) {
+ if (!getLangOpts().CPlusPlus)
+return;
+
aaron.ballman wrote:
> Why is this check disabled for C code
JonasToth updated this revision to Diff 159221.
JonasToth added a comment.
- doc list
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D45444
Files:
clang-tidy/cppcoreguidelines/CMakeLists.txt
clang-tidy/cppcoreguidelines/ConstCorrectnessCheck.cpp
clang-tidy/cppcoreguideline
JonasToth updated this revision to Diff 159224.
JonasToth added a comment.
- revert breaking change in LocalVar matcher.
Reverting the change from `allOf(...)` to `unless(anyOf(..))`. I did not
investigate
the reason for it breaking, because basic logic suggests that transformation
should be cor
JonasToth added inline comments.
Comment at: clang-tidy/cppcoreguidelines/ConstCorrectnessCheck.cpp:147
+ // Example: `int i = 10`, `int i` (will be used if program is correct)
+ const auto LocalValDecl = varDecl(unless(anyOf(
+ isLocal(), hasInitializer(anything()), unles
JonasToth updated this revision to Diff 159225.
JonasToth marked an inline comment as done.
JonasToth added a comment.
- fix typo
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D45444
Files:
clang-tidy/cppcoreguidelines/CMakeLists.txt
clang-tidy/cppcoreguidelines/ConstCorrec
JonasToth updated this revision to Diff 159226.
JonasToth added a comment.
- update ReleaseNotes
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D45444
Files:
clang-tidy/cppcoreguidelines/CMakeLists.txt
clang-tidy/cppcoreguidelines/ConstCorrectnessCheck.cpp
clang-tidy/cppco
JonasToth updated this revision to Diff 159259.
JonasToth added a comment.
- rebase
Repository:
rC Clang
https://reviews.llvm.org/D50307
Files:
tools/clang-rename/clang-rename.py
Index: tools/clang-rename/clang-rename.py
===
This revision was automatically updated to reflect the committed changes.
Closed by commit rL338996: [clang-rename] make clang-rename.py vim integration
python3 compatible (authored by JonasToth, committed by ).
Herald added a subscriber: llvm-commits.
Repository:
rL LLVM
https://reviews.llvm.
JonasToth added a comment.
@hokein Do you think this should go into the 7.0 release?
Repository:
rL LLVM
https://reviews.llvm.org/D50307
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-co
JonasToth added a comment.
In https://reviews.llvm.org/D31370#768684, @baloghadamsoftware wrote:
> The matcher in https://reviews.llvm.org/D33537 could be reused here as well.
> Maybe I should make it common, but it is more complex than any of the common
> matchers.
yes, the matcher seems int
JonasToth added a comment.
I am happy now. But I don't have any authority to allow this patch to land
whatsoever. Who will be the code owner for `clang-doc`? I think the tooling
guys need to accept.
Comment at: tools/clang-doc/ClangDoc.cpp:54
+
+ // TODO: Move set attached t
JonasToth added a comment.
That check is similar to 'hicpp-signed-bitwise' in clang-tidy. Maybe it could
live there and extend it?
Repository:
rC Clang
https://reviews.llvm.org/D41308
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http:
JonasToth added a comment.
What happens if the operator is overloaded outside a class? Is that
allowed/disallowed and could you please mention the guideline on that in the
docs + tests.
Comment at: clang-tidy/fuchsia/OverloadedOperatorCheck.cpp:18
+
+AST_MATCHER(CXXMethodDecl
JonasToth added inline comments.
Comment at: docs/clang-tidy/checks/fuchsia-overloaded-operator.rst:17
+
+See the features disallowed in Fuchsia at
https://fuchsia.googlesource.com/zircon/+/master/docs/cxx.md
juliehockett wrote:
> JonasToth wrote:
> > Could you
JonasToth added a comment.
Is the singleton allowed, that uses a static object in the `instance` method?
https://reviews.llvm.org/D41546
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commi
JonasToth added a comment.
What happens for c++98? I realize that fuchsia is c++14 but we might still
think about not having `constexpr`.
If we just assume c++11 you can do the matching only for it. (`getLangOpts` or
similar, see other checks for it.)
https://reviews.llvm.org/D41546
___
JonasToth added a comment.
@sbenza and/or @klimek did you have time to address the stackoverflow caused
from the ASTMatchers?
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D40737
___
cfe-commits mailing list
cfe-commits@lists.llvm.
JonasToth created this revision.
JonasToth added reviewers: aaron.ballman, hokein, alexfh.
Herald added subscribers: cfe-commits, kbarton, xazax.hun, mgorny, nemanjai,
klimek.
In short macros are discouraged by multiple rules (and sometimes reference
randomly).
This check allows only headerguard
JonasToth updated this revision to Diff 128372.
JonasToth added a comment.
- merge with local repos
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D41648
Files:
clang-tidy/cppcoreguidelines/CMakeLists.txt
clang-tidy/cppcoreguidelines/CppCoreGuidelinesTidyModule.cpp
clang-t
JonasToth updated this revision to Diff 128373.
JonasToth added a comment.
- remove unneeded includes
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D41648
Files:
clang-tidy/cppcoreguidelines/CMakeLists.txt
clang-tidy/cppcoreguidelines/CppCoreGuidelinesTidyModule.cpp
clang
JonasToth added a comment.
In https://reviews.llvm.org/D41648#965432, @aaron.ballman wrote:
> I think this check is going to be extraordinarily chatty. For instance,
> macros are often used to hide compiler details in signatures, such as use of
> attributes. This construct cannot be replaced wi
JonasToth added a comment.
I think it would be more user friendly if the configured list can be a list and
the `|` concatenation is done within your code.
Comment at: clang-tidy/bugprone/UnusedReturnValueCheck.cpp:29
+ llvm::raw_svector_ostream OS(InlinedName);
+ auto Policy
JonasToth added a comment.
Guidelines issue is here:
https://github.com/isocpp/CppCoreGuidelines/issues/1120
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D40854
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.
JonasToth added a comment.
Could you please add a test case with a template that reduces the type to int8
or uint8?
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D41740
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http:/
JonasToth added inline comments.
Comment at: clang-tidy/bugprone/StreamInt8Check.cpp:26
+
hasDeclaration(cxxRecordDecl(hasName("std::basic_ostream",
+hasArgument(1, expr(hasType(hasCanonicalType(
+anyOf(asString("signed char
JonasToth added a comment.
In https://reviews.llvm.org/D41740#968567, @BRevzin wrote:
> In https://reviews.llvm.org/D41740#968134, @JonasToth wrote:
>
> > Could you please add a test case with a template that reduces the type to
> > int8 or uint8?
>
>
> I don't actually know how to do that. I tr
JonasToth added inline comments.
Comment at: clang-tidy/bugprone/StreamInt8Check.h:20
+/// Checks that objects of types int8_t or uint8_t aren't streamed to ostreams,
+/// where the intended behavior is likely to stream them as ints
+///
Missing full stop in comm
JonasToth updated this revision to Diff 128869.
JonasToth added a comment.
- rebase after release for 6.0
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D41648
Files:
clang-tidy/cppcoreguidelines/CMakeLists.txt
clang-tidy/cppcoreguidelines/CppCoreGuidelinesTidyModule.cpp
c
JonasToth updated this revision to Diff 128870.
JonasToth added a comment.
- rebase after release of 6.0
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D40854
Files:
clang-tidy/cppcoreguidelines/CMakeLists.txt
clang-tidy/cppcoreguidelines/CppCoreGuidelinesTidyModule.cpp
cl
JonasToth added a comment.
> Yes, and I'm saying that the guidelines aren't useful for real code bases
> because they restrict more than is reasonable. So while I think the check is
> largely implementing what the guidelines recommend, I think that some of
> these scenarios should be brought ba
JonasToth created this revision.
JonasToth added reviewers: aaron.ballman, alexfh, hokein.
Herald added subscribers: cfe-commits, kbarton, xazax.hun, mgorny, nemanjai,
klimek.
The usage of `goto` is discourage in C++ since forever. This check implements
a warning for every `goto`. Even though the
JonasToth updated this revision to Diff 128898.
JonasToth added a comment.
- fix typos and return type of main in test
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D41815
Files:
clang-tidy/cppcoreguidelines/AvoidGotoCheck.cpp
clang-tidy/cppcoreguidelines/AvoidGotoCheck.h
JonasToth updated this revision to Diff 128899.
JonasToth added a comment.
- [Misc] emphasize goto in warning message
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D41815
Files:
clang-tidy/cppcoreguidelines/AvoidGotoCheck.cpp
clang-tidy/cppcoreguidelines/AvoidGotoCheck.h
JonasToth updated this revision to Diff 128902.
JonasToth added a comment.
- [Misc] reformat
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D41815
Files:
clang-tidy/cppcoreguidelines/AvoidGotoCheck.cpp
clang-tidy/cppcoreguidelines/AvoidGotoCheck.h
clang-tidy/cppcoreguideli
JonasToth added a comment.
In https://reviews.llvm.org/D41815#969626, @xazax.hun wrote:
> A high level comment: while it is very easy to get all the gotos using grep,
> it is not so easy to get all the labels. So for this check to be complete, I
> think it would be useful to also find labels (p
JonasToth added a comment.
> Rather than add a warning for the labels, I would just add a note for the
> label when diagnosing the goto (since the goto has a single target).
That might lead to existing labels without any gotos for them, does it? Maybe
the check could also diagnose labels withou
JonasToth added inline comments.
Comment at: clang-tidy/cppcoreguidelines/AvoidGotoCheck.cpp:22
+ if (getLangOpts().CPlusPlus)
+Finder->addMatcher(gotoStmt().bind("goto"), this);
+}
aaron.ballman wrote:
> aaron.ballman wrote:
> > Are you planning to add the
JonasToth added inline comments.
Comment at: clang-tidy/cppcoreguidelines/AvoidGotoCheck.cpp:22
+ if (getLangOpts().CPlusPlus)
+Finder->addMatcher(gotoStmt().bind("goto"), this);
+}
aaron.ballman wrote:
> JonasToth wrote:
> > aaron.ballman wrote:
> > > aaron
JonasToth updated this revision to Diff 129433.
JonasToth added a comment.
I enhanced the check to do more:
- check that jumps will only be forward. AFAIK that is required in all
sensefull usecases of goto, is it?
- additionally check for gotos in nested loops. These are not diagnosed if the
ju
JonasToth added a comment.
After long inactivity (sorry!) i had a chance to look at it again:
switch(i) {
case 0:;
case 1:;
case 2:;
...
}
does *NOT* lead to the stack overflow. This is most likely an issue in the AST:
https://godbolt.org/g/vZw2BD
Empty case labels do nest, an empty
JonasToth added a comment.
The check is related to:
https://github.com/isocpp/CppCoreGuidelines/blob/master/CppCoreGuidelines.md#cp44-remember-to-name-your-lock_guards-and-unique_locks
If you want, you can add an alias into the `cppcoreguideline` module.
Repository:
rCTE Clang Tools Extra
h
JonasToth added inline comments.
Comment at: docs/ReleaseNotes.rst:125
+
+ Flags functions exceeding this number of variables declared in the body.
+
I would rephrase this a little to:
```
Flags function bodies exceeding this number of declared variables.
```
JonasToth added a comment.
Looks fine to me, but Aaron or someone else must accept :)
Just out of curiosity: The decomposition visit is for structured binding and
the binding visit for range based for? Are there other places where binding
occurs? For example in some template stuff? Just asking
JonasToth added a comment.
> The comment i added is actually wrong.
> The range-based for just works anyway.
> The VisitBindingDecl() is needed to get all the 'variables' declared in
> structured binding.
> But as you can see in https://godbolt.org/g/be6Juf, the BindingDecl's are
> wrapped in
JonasToth added inline comments.
Comment at: docs/ReleaseNotes.rst:125
+
+ Flags functions exceeding this number of variables declared in the body.
+
lebedev.ri wrote:
> JonasToth wrote:
> > I would rephrase this a little to:
> >
> > ```
> > Flags function bod
JonasToth added a subscriber: rsmith.
JonasToth added a comment.
Good news: The stack overflow seems to be fixed, by the patch r326624 from
@rsmith (Thank you very much!).
Bad news: The check does not do its job anymore I try to fix the
functionality problems.
Repository:
rCTE Clang Tool
JonasToth updated this revision to Diff 138849.
JonasToth added a comment.
- get the check working again
- enable the big test
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D40737
Files:
clang-tidy/hicpp/CMakeLists.txt
clang-tidy/hicpp/HICPPTidyModule.cpp
clang-tidy/hicpp
JonasToth updated this revision to Diff 138850.
JonasToth added a comment.
- reorder check in release notes to get it in alphabetical order
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D40737
Files:
clang-tidy/hicpp/CMakeLists.txt
clang-tidy/hicpp/HICPPTidyModule.cpp
cla
JonasToth added a comment.
I think the check is ready to land. I did check run it over LLVM and found some
interesting code parts that could benefit. I extracted all the warnings and
sorted them into the categories `missing default/covered
codepath(uncovered.txt)`, `better use if/else(better_if
JonasToth updated this revision to Diff 138851.
JonasToth added a comment.
- remove spurious debug iostream
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D40737
Files:
clang-tidy/hicpp/CMakeLists.txt
clang-tidy/hicpp/HICPPTidyModule.cpp
clang-tidy/hicpp/MultiwayPathsCover
JonasToth added a comment.
@shuaiwang I can commit it in 2 hours for you if you want, after my lecture.
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D45679
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.o
JonasToth added a comment.
It might be the case, that the test is run with -no-stdinc (or similar),
so the standard library is not available.
Am 13.06.2018 um 23:08 schrieb Shuai Wang via Phabricator:
> shuaiwang added a comment.
>
> In https://reviews.llvm.org/D45679#1131115, @aaron.ballman wr
JonasToth added inline comments.
Comment at: clang-tidy/performance/UnnecessaryCopyInitialization.cpp:65-66
has(varDecl(hasLocalStorage(),
- hasType(matchers::isExpensiveToCopy()),
+ hasTy
JonasToth added a comment.
Hi ymandel,
welcome to the LLVM community and thank you very much for working on that check!
If you have any questions or other issues don't hesitate to ask ;)
Comment at: clang-tidy/readability/ConstValueReturnCheck.cpp:25
+
+// Finds the location
JonasToth added a comment.
In https://reviews.llvm.org/D46027#1259989, @ZaMaZaN4iK wrote:
> What is the status of the PR?
I believe xazax doesnt have time to work further, you can commandeer if you
want :)
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D46027
_
JonasToth added a comment.
LG in principle, just the SmallVec thing could be done if you agree. I don't
insist on it, but it looks like a performance benefit to me.
Comment at: clang-tidy/performance/UnnecessaryCopyInitialization.h:41
ASTContext
JonasToth added a comment.
This looks reasonable to me but could you please explain a bit what the issue
was in the bugreport? So a very deep hierarchy causing the problem makes sense,
but why was "ignoring first" the difference maker?
Would it make sense to add a warning in the documentation th
JonasToth accepted this revision.
JonasToth added a comment.
This revision is now accepted and ready to land.
Did you verify it actually works?
Otherwise LGTM and because its a bug-fix you can commit and other concerns can
be done post-commit.
Comment at: test/clang-tidy/check
JonasToth added a comment.
I see, probably not worth it. Its one-time effort anyway right?
LGTM
https://reviews.llvm.org/D52727
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
JonasToth added a comment.
In https://reviews.llvm.org/D53187#1263294, @baloghadamsoftware wrote:
> I think that with this optimization it is not so expensive anymore. I do not
> think it was an endless loop in the bugreport but it was insufferable
> execution time. Maybe we could speed it up a
JonasToth added a comment.
> Unfortunately, I won't be able to review the code in the upcoming few weeks
> as I caught a cold and I'm trying to get better before the LLVM Meeting, so
> if there is anyone else interested in reviewing proposed changes please feel
> free to jump in.
Thank you ver
JonasToth added a comment.
> Thousands? After the query optimization the max was 173, and that only for a
> single function. The next number was 64.
See https://bugs.llvm.org/attachment.cgi?id=20963
I did not run again with your patch. But even 173 and 64 seem like a lot and
might be worth opti
JonasToth updated this revision to Diff 169389.
JonasToth added a comment.
- fix headline in doc
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D51949
Files:
clang-tidy/readability/CMakeLists.txt
clang-tidy/readability/IsolateDeclarationCheck.cpp
clang-tidy/readability/Iso
JonasToth added inline comments.
Comment at: clang-tidy/readability/ConstValueReturnCheck.cpp:25
+
+// Finds the location of the relevant "const" token in `Def`.
+llvm::Optional findConstToRemove(
ymandel wrote:
> JonasToth wrote:
> > s/"const"/`const`
> here and
JonasToth added a comment.
In https://reviews.llvm.org/D52892#1263324, @aaron.ballman wrote:
> I like the thrust of this check, but we already have the
> readability-magic-numbers check and I think that this functionality would fit
> naturally there. That check finds numerical constants and rec
JonasToth updated this revision to Diff 169402.
JonasToth marked 3 inline comments as done.
JonasToth added a comment.
- add tests and adjust doc
- one more test case
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D41648
Files:
clang-tidy/cppcoreguidelines/CMakeLists.txt
cla
JonasToth added a comment.
Updated
Comment at: clang-tidy/cppcoreguidelines/MacroUsageCheck.cpp:78
+"function-like macro used; consider a 'constexpr' template function";
+ if (Info->isVariadic())
+DiagnosticMessage = "variadic macro used; consider using a 'constexp
JonasToth updated this revision to Diff 169403.
JonasToth added a comment.
- remove unused enum in header file, no idea what i intended to do with it :D
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D41648
Files:
clang-tidy/cppcoreguidelines/CMakeLists.txt
clang-tidy/cppcor
JonasToth added inline comments.
Comment at: test/clang-tidy/misc-non-private-member-variables-in-classes.cpp:1
+// RUN: %check_clang_tidy -check-suffix=NONPRIVATE %s
misc-non-private-member-variables-in-classes %t
+// RUN: %check_clang_tidy -check-suffix=NONPRIVATE %s
misc-non
JonasToth updated this revision to Diff 169410.
JonasToth added a comment.
- make the logic with variadic clear
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D41648
Files:
clang-tidy/cppcoreguidelines/CMakeLists.txt
clang-tidy/cppcoreguidelines/CppCoreGuidelinesTidyModule.c
JonasToth added a comment.
The patch does not apply clean, could you please take a look at it?
Am 12.10.2018 um 17:21 schrieb Whisperity via Phabricator:
> whisperity added a comment.
>
> @aaron.ballman Neither I nor @Charusso has commit rights, could you please
> commit this in our stead?
>
JonasToth added inline comments.
Comment at: clang-tidy/readability/ConstValueReturnCheck.cpp:25
+
+// Finds the location of the relevant "const" token in `Def`.
+llvm::Optional findConstToRemove(
ymandel wrote:
> JonasToth wrote:
> > ymandel wrote:
> > > JonasTo
JonasToth added a comment.
Committed with https://reviews.llvm.org/rCTE344374
https://reviews.llvm.org/D45050
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
JonasToth accepted this revision.
JonasToth added a comment.
This revision is now accepted and ready to land.
In https://reviews.llvm.org/D53187#1264415, @baloghadamsoftware wrote:
> I think further optimization steps should be done is separate patches.
> However, this is the biggest step.
Agr
JonasToth added inline comments.
Comment at: docs/ReleaseNotes.rst:113
+
+- New alias :doc:`cppcoreguidelines-non-private-member-variables-in-classes
+
`
lebedev.ri wrote:
> Eugene.Zelenko wrote:
> > Please move new alias after new checks.
> You mean after all
JonasToth added inline comments.
Comment at: clang-tidy/readability/ConstValueReturnCheck.cpp:64
+
+ // Fix the definition.
+ llvm::Optional Loc = findConstToRemove(Def, Result);
ymandel wrote:
> JonasToth wrote:
> > ymandel wrote:
> > > JonasToth wrote:
> > >
JonasToth added a comment.
Unfortunatly this check does not compile on some ARM platforms. It seems that a
matcher exceeds the recursion limit in template instantations.
http://lab.llvm.org:8011/builders/clang-cmake-armv7-quick/builds/4405/steps/build%20stage%201/logs/stdio
I will revert the ch
JonasToth added inline comments.
Comment at: clang-tidy/readability/UppercaseLiteralSuffixCheck.cpp:94
+ // Else, find matching suffix, case-*insensitive*ly.
+ for (const auto &PotentialNewSuffix : NewSuffixes) {
+if (!OldSuffix.equals_lower(PotentialNewSuffix))
---
JonasToth added a comment.
I am in contact with the guy that actually discovered this breakage. It seems
to be a weird and hard to reproduce error, but happens on x86 as well. So it is
a compile/header something combination that results in the error. I am pretty
sure that will take a while unti
JonasToth added inline comments.
Comment at: clang-tidy/abseil/DurationFactoryFloatCheck.cpp:34
+llvm::APSInt ApInt(/*BitWidth=*/64, /*isUnsigned=*/false);
+ApInt = static_cast(value);
+if (is_negative)
Wouldn't it make more sense to use `std::uint64_
JonasToth added a comment.
Thank you for the restructurings, i think the code is now way clearer and the
check close to being done (at least from my side :)).
Could you please mark all notes you consider done as done? Right now i am a bit
lost on what to track on, as the locations of the notes a
JonasToth added inline comments.
Comment at: clang-tidy/abseil/DurationFactoryFloatCheck.cpp:69
+ // Macros are ignored.
+ if (Arg->getBeginLoc().isMacroID())
+return;
hwright wrote:
> JonasToth wrote:
> > maybe `assert` instead, as your comment above sugge
JonasToth added a comment.
In https://reviews.llvm.org/D53339#1269998, @hwright wrote:
> Ping.
>
> What are the next steps here?
Please mark all comments you consider resolved as 'Done', i think alex already
kinda accepted it, but given there were more comments he should take another
look.
JonasToth accepted this revision.
JonasToth added a comment.
This revision is now accepted and ready to land.
LGTM, but could you please add a short notice in the release notes?
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D53454
_
JonasToth added inline comments.
Comment at: clang-tidy/cppcoreguidelines/MacroUsageCheck.h:34-35
+ void registerPPCallbacks(CompilerInstance &Compiler) override;
+ void warnMacro(const MacroDirective *MD);
+ void warnNaming(const MacroDirective *MD);
+
aaron.
JonasToth updated this revision to Diff 170470.
JonasToth marked 4 inline comments as done.
JonasToth added a comment.
- address review nits
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D41648
Files:
clang-tidy/cppcoreguidelines/CMakeLists.txt
clang-tidy/cppcoreguidelines/
JonasToth updated this revision to Diff 170471.
JonasToth added a comment.
- add missing private
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D41648
Files:
clang-tidy/cppcoreguidelines/CMakeLists.txt
clang-tidy/cppcoreguidelines/CppCoreGuidelinesTidyModule.cpp
clang-tidy
201 - 300 of 1402 matches
Mail list logo
