Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

CxxPreprocessor: cache forced includes, forced & global defines #1665

Merged
merged 6 commits into from
Jan 17, 2019
Merged

CxxPreprocessor: cache forced includes, forced & global defines #1665

merged 6 commits into from
Jan 17, 2019

Conversation

ivangalkin
Copy link
Contributor

@ivangalkin ivangalkin commented Jan 11, 2019 •
edited
Loading

Also:

apply correct order macro definitions

  1. standard macro values (e.g. __LINE__)
  2. configured defines (e.g. -D__LINE__=123)
  3. forced include files (e.g. header contains #define __LINE__ 456)

-> 2 overrides 1
-> 3 overrides 2 and 1

This change is Reviewable

@ivangalkin ivangalkin added this to the 1.2.2 milestone Jan 11, 2019
@ivangalkin ivangalkin self-assigned this Jan 11, 2019
@ivangalkin ivangalkin requested a review from guwirth January 11, 2019 20:09
@ivangalkin
Copy link
Contributor Author

@guwirth I found out, that you marked sonar.cxx.jsonCompilationDatabase as deprecated (see https://github.com/SonarOpenCommunity/sonar-cxx/wiki/Supported-configuration-properties/_compare/2a3859fbbe6e030b32a06245b61b0764310b8807...eef90bd839398469d58d4495de2fc803d90e051b). I strongly disagree with the deprecation. At the moment JSON compilation database it the only

  • convenient and
  • compiler independent way

to add the includes and defines.

SonarCFamily also uses some similar but proprietary solution: intercept the build and store the compiler arguments into the JSON database.

@guwirth
Copy link
Collaborator

guwirth commented Jan 12, 2019 •
edited
Loading

@ivangalkin maybe that is a misunderstanding.

Release notes:

  • Json compilation database support sonar.cxx.jsonCompilationDatabase is also experimental only
    sonar.cxx.scanOnlySpecifiedSources is no more supported. There were conflicts with sonar.sources and sonar.tests.

The problem I see is that we have too many ways to handle macros and includes. Having different input formats is ok for me but internally we should handle it in an unique way. That was the reason for #1365, #1325 and #1326.

There seems to be also some open questions like #1215.

I had also less feedback that someone is using it. That’s the reason why the functionality is marked experimental.

@ivangalkin
Copy link
Contributor Author

@guwirth right now the page "Supported configuration properties" looks as following:

sonar.cxx.jsonCompilationDatabase

Scope: project
Default: ---
History: no more supported with v1.0.0+

Enable Sonar C++ analysis to utilize JSON compilation database support. Specifies file to be used as JSON compilation database. JSON compilation database is used to improve C++ symbol and include directory knowledge. Sonar C++ plugin also supports extension to allow importing of used compiler's internal symbols and includes.

For more information please see: JSON compilation database format specification support

Example:
sonar.cxx.scanOnlySpecifiedSources

Scope: project
Default: False

When using JSON compilation database support limit Sonar C++ plugin analysis to specified files in JSON compilation database. This setting can be used to limit analysis to only compiled files.

Example:

According to you comment, the deprecation note "History: no more supported with v1.0.0+" should be moved from sonar.cxx.jsonCompilationDatabase to sonar.cxx.scanOnlySpecifiedSources?

@ivangalkin ivangalkin changed the title CxxPreprocessor: cache forced & global defines CxxPreprocessor: cache forced includes, forced & global defines Jan 12, 2019
@guwirth
Copy link
Collaborator

guwirth commented Jan 13, 2019

According to you comment, the deprecation note "History: no more supported with v1.0.0+" should be moved from sonar.cxx.jsonCompilationDatabase to sonar.cxx.scanOnlySpecifiedSources?

@ivangalkin done

guwirth
guwirth reviewed Jan 14, 2019
View reviewed changes
Copy link
Collaborator

@guwirth guwirth left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ivangalkin thanks a lot for this - think it's really an improvement. I added some comments. I*m not sure where the right place for a cache is: parser or configuration? See comments in #1365, #1325 and #1326.

cxx-squid/src/main/java/org/sonar/cxx/preprocessor/CxxPreprocessor.java Outdated Show resolved Hide resolved
cxx-squid/src/main/java/org/sonar/cxx/preprocessor/CxxPreprocessor.java Outdated Show resolved Hide resolved
cxx-squid/src/main/java/org/sonar/cxx/preprocessor/CxxPreprocessor.java Outdated Show resolved Hide resolved
cxx-squid/src/main/java/org/sonar/cxx/preprocessor/CxxPreprocessor.java Outdated
}

// set standard macros
getMacros().putAll(forcedMacros);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this the right order? First standard macros and then forced? Think should be possible to overwrite standard macros with forced.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you are absolutely right: we can use the following analogue behavior:

  1. standard macro values (e.g. __LINE__)
  2. configured defines (e.g. -D__LINE__=123)
  3. forced include files (e.g. header contains #define __LINE__ 456)

  -> 2 overrides 1
  -> 3 overrides 2 and 1

I've reordered the priorities and have written the tests. See

  • CxxLexerWithPreprocessingTest.defaultMacros
  • CxxLexerWithPreprocessingTest.configuredDefinesOverrideDefaultMacros
  • CxxLexerWithPreprocessingTest.forcedIncludesOverrideConfiguredDefines

cxx-squid/src/main/java/org/sonar/cxx/preprocessor/CxxPreprocessor.java Outdated Show resolved Hide resolved
cxx-squid/src/main/java/org/sonar/cxx/preprocessor/CxxPreprocessor.java
//Create macros to replace C++ keywords when parsing C files
getMacros().putAll(Macro.COMPATIBILITY_MACROS);
fixedMacros.disable(CPLUSPLUS);
getMacros().disable(Macro.CPLUSPLUS);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

think there are more macros different between C and C++?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the map Macro.STANDARD_MACROS_IMPL contains only two C++ specific macros:

  • __cplusplus
  • __has_include (since C++17)

so theoretically we have to disable __has_include to. BUT

  1. I tested GCC in the ANSI-C mode and it still provides this define (https://coliru.stacked-crooked.com/a/30a53f2c2a9a323d)
  2. Our implementation of __has_include is hard-wired in the grammar and the expansion of __has_include() is not restricted to C++. So even if #ifdef __has_include evaluates to 0, we still can't prevent __has_include(...) from evaluation.
  3. This C vs C++ was not scope of my change, so please let's consider to change it in other PR

cxx-squid/src/main/java/org/sonar/cxx/preprocessor/CxxPreprocessor.java Show resolved Hide resolved
@ivangalkin
apply review comments
08b548e 
* renaming
* apply correct order macro definitions

  1. standard macro values (e.g. __LINE__)
  2. configured defines (e.g. -D__LINE__=123)
  3. forced include files (e.g. header contains #define __LINE__ 456)

  -> 2 overrides 1
  -> 3 overrides 2 and 1
guwirth
guwirth reviewed Jan 15, 2019
View reviewed changes
cxx-squid/src/main/java/org/sonar/cxx/preprocessor/CxxPreprocessor.java Outdated
parseForcedIncludes();
predefinedUnitMacros = new HashMap<>(unitMacros.getHighPrioMap());
} finally {
unitMacros = null;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ivangalkin is here getMacros().setHighPrio(false) missing?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

no, in this case we don't have to disable the high-prio mode

  1. unitMacros = new MapChain<>(); -> make getMacro() return unitMacros
  2. fill the map getMacro() directly and (more important) indirectly through parseForcedIncludes()
  3. all precompiled macros for compile units are now in unitMacros.getHighPrioMap(); we copy them into predefinedUnitMacros
  4. unitMacros = null; -> destroy the unitMacros; they won't be filled from constructor anymore, no need to toggle the high-prio mode

guwirth
guwirth reviewed Jan 15, 2019
View reviewed changes
cxx-squid/src/main/java/org/sonar/cxx/preprocessor/CxxPreprocessor.java
@@ -127,34 +173,56 @@ public CxxPreprocessor(SquidAstVisitorContext<Grammar> context,

pplineParser = CppParser.create(conf);

final List<String> configuredDefines = conf.getDefines();
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ivangalkin ctor code is difficult to read. Maybe you can split it into smaller methods.

Copy link
Contributor Author

@ivangalkin ivangalkin Jan 15, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes, I'll do some refactoring;

@ivangalkin
apply review comments (2)
e984eea 
* refactor CxxPreprocessor::CxxPreprocessor()
@ivangalkin
fix typo
889b7c6
guwirth
guwirth approved these changes Jan 16, 2019
View reviewed changes
Copy link
Collaborator

@guwirth guwirth left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ivangalkin thanks looks good now.

@guwirth guwirth merged commit 19be9a5 into SonarOpenCommunity:master Jan 17, 2019
@guwirth guwirth mentioned this pull request Feb 8, 2019
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@guwirth guwirth guwirth approved these changes

Assignees

@ivangalkin ivangalkin

Labels
enhancement
Milestone
1.2.2
Development

Successfully merging this pull request may close these issues.
2 participants
@ivangalkin @guwirth