search

Ericsson / clang

Cross Translation Unit analysis capability for Clang Static Analyzer. (Fork of official clang at http://llvm.org/git/clang)
http://clang.llvm.org/
Other
15 stars 10 forks source link

Add analyzer option to limit the number of TUs imported #619

Closed gamesh411 closed 5 years ago

gamesh411 commented 5 years ago

I have run into a dependency issue. If I store the TU threshold as an AnalyzerOption, then I have to link with libStaticAnalyzerCore (see CMakeLists.txt change). But this introduces a cyclic dependency. I see a couple of solutions to this problem:

a) put the option elsewhere, for example as a general frontend-option b) make the check elsewhere, for example inside some analyzer-specific manager class c) try to cut the dependency by introducing intermediary libs

What are your thoughts?

martong commented 5 years ago

I have run into a dependency issue. If I store the TU threshold as an AnalyzerOption, then I have to link with libStaticAnalyzerCore (see CMakeLists.txt change). But this introduces a cyclic dependency. I see a couple of solutions to this problem:

a) put the option elsewhere, for example as a general frontend-option b) make the check elsewhere, for example inside some analyzer-specific manager class c) try to cut the dependency by introducing intermediary libs

What are your thoughts?

I prefer option a). Because CrossTU is ought to be independent from the analyzer, that is more generic.

Szelethus commented 5 years ago

Shouldn't this be an analyzer config option instead? I am aware of several CTU options that are just that, why is this a frontend option?

martong commented 5 years ago

Shouldn't this be an analyzer config option instead? I am aware of several CTU options that are just that, why is this a frontend option?

Yes this should be an analyzer option, but the first attempt somehow created a lib dependency failure. So, Endre is working on the second attempt where he will extend the parameters of CrossTranslationUnitContext::getCrossTUDefinition like we do with DisplayCTUProgress.

Szelethus commented 5 years ago

Shouldn't this be an analyzer config option instead? I am aware of several CTU options that are just that, why is this a frontend option?

Yes this should be an analyzer option, but the first attempt somehow created a lib dependency failure.

The reason probably was that my analyzer config changes are not live on clang7 -- the solution would be to leave the option's getter method in-class, I believe.

dkrupp commented 5 years ago

deadline for this is 31st of March!

gamesh411 commented 5 years ago

Reorganized parameter handling, added unit test and some documentation.

Szelethus commented 5 years ago

Sorry for missing out on the discussion. I think adding this as an analyzer-config flag WOULD be preferred on clang 8.0.0, but since we can't list and verify these options in 7.0.0., let's not shoot ourselves in the foot with them.

LGTM regarding option placement, I can't say much about the rest of the patch.