This revision was automatically updated to reflect the committed changes.
Closed by commit rL343528: [clang-tidy] Build it even without static analyzer
(authored by steveire, committed by ).
Herald added a subscriber: llvm-commits.
Changed prior to commit:
steveire added a comment.
I'll take care of it in a few days and commit it myself. Thanks!
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D52334
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
aaron.ballman reopened this revision.
aaron.ballman added a comment.
This revision is now accepted and ready to land.
In https://reviews.llvm.org/D52334#1250439, @aaron.ballman wrote:
> I've commit as r343415, thank you for the patch!
I've reverted in r343418 as the commit broke some bots.
aaron.ballman closed this revision.
aaron.ballman added a comment.
I've commit as r343415, thank you for the patch!
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D52334
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
aaron.ballman accepted this revision.
aaron.ballman added a comment.
This revision is now accepted and ready to land.
LGTM! If you need this merged on your behalf, I can do so tomorrow or Monday,
unless someone else gets to it first.
Repository:
rCTE Clang Tools Extra
steveire marked an inline comment as done.
steveire added a comment.
> Once I'm happy, I will accept it. If @alexfh has further comments, then they
> can be addressed at that time (pre or post commit).
That's very reasonable, thanks.
Repository:
rCTE Clang Tools Extra
steveire updated this revision to Diff 167619.
steveire added a comment.
Doc fix
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D52334
Files:
CMakeLists.txt
clang-tidy/CMakeLists.txt
clang-tidy/ClangTidy.cpp
clang-tidy/plugin/CMakeLists.txt
steveire updated this revision to Diff 167618.
steveire added a comment.
Format docs
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D52334
Files:
CMakeLists.txt
clang-tidy/CMakeLists.txt
clang-tidy/ClangTidy.cpp
clang-tidy/plugin/CMakeLists.txt
aaron.ballman added a comment.
In https://reviews.llvm.org/D52334#1250166, @steveire wrote:
> > I think a reasonable place would be docs/clang-tidy/index.rst in the
> > "Getting Involved" area.
>
> Thanks, that's at least actionable, but not very specific. I've added docs
> now. If they don't
steveire marked 2 inline comments as done.
steveire added a comment.
> I think a reasonable place would be docs/clang-tidy/index.rst in the "Getting
> Involved" area.
That's at least actionable, but not very specific. I've added docs now. If they
don't say what you want them to say, then
steveire updated this revision to Diff 167615.
steveire added a comment.
Herald added a subscriber: arphaman.
Docs
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D52334
Files:
CMakeLists.txt
clang-tidy/CMakeLists.txt
clang-tidy/ClangTidy.cpp
aaron.ballman added a comment.
In https://reviews.llvm.org/D52334#1249875, @steveire wrote:
> @aaron.ballman Happy to add a note to the docs, but your request leaves me
> wondering where?
>
> AFAIK, the fact that `clang-tidy` wasn't built at all when using
> `CLANG_ENABLE_STATIC_ANALYZER` is
steveire marked 2 inline comments as done.
steveire added a comment.
@aaron.ballman Happy to add a note to the docs, but your request leaves me
wondering where?
AFAIK, the fact that `clang-tidy` wasn't built at all when using
`CLANG_ENABLE_STATIC_ANALYZER` is not documented, so I can't just
steveire updated this revision to Diff 167584.
steveire added a comment.
Whitespace etc
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D52334
Files:
CMakeLists.txt
clang-tidy/CMakeLists.txt
clang-tidy/ClangTidy.cpp
clang-tidy/plugin/CMakeLists.txt
aaron.ballman added a comment.
I think that we need some sort of developer documentation change that explains
1) that this option exists when configuring clang-tidy, and 2) that this
impacts the MPI module as well as the static analyzer.
Comment at:
JonasToth added a comment.
In https://reviews.llvm.org/D52334#1247809, @steveire wrote:
> @JonasToth Once again - `clangStaticAnalyzerCheckers` is not `clangAnalysis`.
> Also, that commit changes the `mpi` plugin, which is excluded by this patch.
I pinged because of the MPI thing, i did not
steveire added a comment.
@JonasToth Once again - `clangStaticAnalyzerCheckers` is not `clangAnalysis`.
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D52334
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
JonasToth added a comment.
There has been a commit that seems related: https://reviews.llvm.org/rL343168
@steveire could you please take a look at how it affects this patch?
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D52334
___
JonasToth added a comment.
Please wait for explicit approval from @alexfh
Am 26.09.2018 um 13:05 schrieb Stephen Kelly via Phabricator:
> steveire added a comment.
>
> Can I go ahead and merge this now?
>
> Repository:
>
> rCTE Clang Tools Extra
>
> https://reviews.llvm.org/D52334
lebedev.ri added a comment.
In https://reviews.llvm.org/D52334#1246356, @steveire wrote:
> Can I go ahead and merge this now?
In https://reviews.llvm.org/D52334#1242813, @lebedev.ri wrote:
> `#ifdef` hell is usually messy and is a source of problems.
> May i ask what is the motivation for
steveire added a comment.
Can I go ahead and merge this now?
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D52334
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
steveire marked an inline comment as done.
steveire added inline comments.
Comment at: test/CMakeLists.txt:69
-clang-tidy
-)
-endif()
+ clang-tidy
+ )
alexfh wrote:
> There are some clang-tidy tests for the static analyzer integration (at least
>
steveire updated this revision to Diff 166825.
steveire added a comment.
Enable tests
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D52334
Files:
CMakeLists.txt
clang-tidy/CMakeLists.txt
clang-tidy/ClangTidy.cpp
clang-tidy/plugin/CMakeLists.txt
alexfh added a comment.
> ! In https://reviews.llvm.org/D52334#1242955, @JonasToth wrote:
> ... to me it makes sense to have clang-tidy without CSA.
Yep, it seems reasonable.
Comment at: test/CMakeLists.txt:69
-clang-tidy
-)
-endif()
+ clang-tidy
+ )
steveire updated this revision to Diff 166644.
steveire marked 2 inline comments as done.
steveire added a comment.
Handle tests
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D52334
Files:
CMakeLists.txt
clang-tidy/CMakeLists.txt
clang-tidy/ClangTidy.cpp
steveire added a comment.
Actually, I had not run the tests. Thanks for the reminder there. I extended
the patch to enable the tests even if CSA is not available.
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D52334
___
JonasToth added a comment.
In https://reviews.llvm.org/D52334#1242881, @steveire wrote:
> Thanks, that at least makes it more obvious where you are getting confused.
>
> See `tools/clang/lib/CMakeLists.txt`. It contains:
>
> add_subdirectory(Analysis)
> # ...
>
steveire added a comment.
Thanks, that at least makes it more obvious where you are getting confused.
See `tools/clang/lib/CMakeLists.txt`. It contains:
add_subdirectory(Analysis)
...
===
if(CLANG_ENABLE_STATIC_ANALYZER)
add_subdirectory(StaticAnalyzer)
endif()
1. That is: Analysis and
JonasToth added a comment.
>> Some of the clang-tidy stuff relies on Analysis/* from clang as well, e.g.
>> the CFG class. Is this still included in builds with CSA off?
>
> The `Analysis` includes are ifdef'd out in the patch. Have you read the patch?
Yes I did read the patch.
steveire added a comment.
> But that clang-tidy is deactivated totally makes that impossible :)
Yes. That should be clear by reading the patch.
> Some of the clang-tidy stuff relies on Analysis/* from clang as well, e.g.
> the CFG class. Is this still included in builds with CSA off?
The
JonasToth added a comment.
In https://reviews.llvm.org/D52334#1242811, @steveire wrote:
> @JonasToth Sorry, I don't know what's unclear. I'm so surprised by your
> question that I think maybe I'm missing something. I thought the commit
> message and the patch itself are clear. Am I missing
lebedev.ri added a comment.
`#ifdef` hell is usually messy and is a source of problems.
May i ask what is the motivation for this change?
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D52334
___
cfe-commits mailing list
steveire added a comment.
@JonasToth Sorry, I don't know what's unclear. I'm so surprised by your
question that I think maybe I'm missing something. I thought the commit message
and the patch itself are clear. Am I missing something?
Currently you can only build clang-tidy if you build the
JonasToth added a comment.
Do builds even work properly if the CSA is not build right now?
Comment at: clang-tidy/tool/CMakeLists.txt:32
clangTidyModernizeModule
- clangTidyMPIModule
clangTidyObjCModule
Is the MPI module remove from the list of
34 matches
Mail list logo
