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.

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

templates: separate interface from implementation (.inl, .tcc) #2094

Closed
pcasenove opened this issue Apr 1, 2021 · 27 comments
Closed

templates: separate interface from implementation (.inl, .tcc) #2094

pcasenove opened this issue Apr 1, 2021 · 27 comments

Comments

@pcasenove
Copy link

pcasenove commented Apr 1, 2021

Hello,
I'm looking for some guidance on how to configure sonar-cxx on a specific setup with template implementation.
Here is the setup:

  • File header.h : template header
  • File header.hpp: implementation of the header

In header.h, I have:

#ifndef HEADER
#define HEADER
[...]
/// @cond INTERNAL
#define HEADER_IMPL
#include "header.hpp"
#undef HEADER_IMPL
/// @endcond

#endif

In header.hpp, I have:
#ifdef HEADER_IMPL

The issue I encounter is quite simple: when I launch sonar-scanner, it doesn't parse header.hpp at all.
If I add sonar.cxx.defines=HEADER_IMPL to the command line (or comment it), it is correctly parsed.

But this is not scalable: I have multiple headers files and can't update the sonar.cxx.defines to list all.

What is the best option or way to use sonar-scanner is such case ?

Thanks in advance for you help,

@guwirth
Copy link
Collaborator

guwirth commented Apr 2, 2021

Hello @pcasenove,

Regards,

@pcasenove
Copy link
Author

Hi,
Sorry, I totally to give you this info...
sonar-cxx 1.3.3 on SQ 7.9.6 LTS

Regards,

@guwirth
Copy link
Collaborator

guwirth commented Apr 2, 2021

@pcasenove
Copy link
Author

pcasenove commented Apr 2, 2021

With debug info on, I get:

13:00:11.901 DEBUG: global settings for: 'header.hpp'
13:00:11.901 DEBUG: Parse 'header.hpp' as C++ file
13:00:11.906 DEBUG: API File: header.hpp
13:00:11.906 DEBUG: Header file suffixes: [.hxx, .hpp, .hh, .h]
13:00:11.906 DEBUG: finished preprocessing 'header.hpp'
13:00:11.906 DEBUG: API File: header.hpp
13:00:11.906 DEBUG: Header file suffixes: [.h, .hh, .hpp, .H]
13:00:11.909 DEBUG: 'header.hpp' generated metadata with charset 'UTF-8'
13:00:11.909 DEBUG: Not enough content in 'header.hpp' to have CPD blocks, it will not be part of the duplication detection
13:00:11.909 DEBUG: CxxFileLinesVisitor: 'header.hpp'
13:00:11.909 DEBUG:    lines:           '405'
13:00:11.909 DEBUG:    executableLines: '[]'
13:00:11.910 DEBUG:    linesOfCode:     '[]'
13:00:11.910 DEBUG:    linesOfComments: '[]'

When I remove the ifdef (or provide the define HEADER_IMPL through sonar.cxx.defines) :
[...]

13:03:44.926 DEBUG: CxxFileLinesVisitor: header.hpp'
13:03:44.926 DEBUG:    lines:           '405'
13:03:44.926 DEBUG:    executableLines: '[388, 389, 262, 264, 396, 397, 275, 276, 150, 151, 281, 163, 164, 295, 297, 171, 172, 306, 52, 53, 184, 185, 314, 318, 320, 323, 326, 328, 330, 77, 337, 338, 340, 88, 349, 351, 353, 357, 232, 360, 362, 237, 371, 372, 119, 249, 122, 250, 123, 380, 381]'
13:03:44.926 DEBUG:    linesOfCode:     '[51, 52, 53, 59, 60, 65, 66, 67, 68, 69, 70, 71, 73, 74, 75, 76, 77, 78, 79, 80, 81, 82, 83, 84, 85, 86, 87, 88, 89, 90, 91, 92, 98, 99, 104, 105, 106, 107, 108, 109, 110, 112, 113, 115, 116, 117, 118, 119, 121, 122, 123, 124, 125, 126, 127, 139, 140, 145, 146, 147, 148, 149, 150, 151, 157, 158, 159, 160, 161, 162, 163, 164, 170, 171, 172, 178, 179, 180, 181, 182, 183, 184, 185, 192, 193, 196, 197, 198, 199, 200, 204, 205, 206, 207, 208, 209, 213, 214, 218, 219, 220, 221, 222, 223, 230, 231, 232, 233, 234, 235, 236, 237, 238, 239, 240, 241, 242, 243, 244, 245, 246, 247, 248, 249, 250, 256, 257, 259, 260, 261, 262, 263, 264, 265, 266, 267, 268, 269, 270, 271, 272, 273, 274, 275, 276, 279, 280, 281, 287, 288, 290, 291, 292, 293, 294, 295, 296, 297, 298, 299, 300, 301, 302, 303, 304, 305, 306, 309, 310, 312, 313, 314, 316, 317, 318, 319, 320, 323, 324, 325, 326, 328, 330, 333, 334, 336, 337, 338, 340, 343, 344, 345, 347, 348, 349, 350, 351, 352, 353, 356, 357, 358, 359, 360, 362, 366, 367, 368, 369, 370, 371, 372, 375, 376, 377, 378, 379, 380, 381, 384, 385, 386, 387, 388, 389, 392, 393, 394, 395, 396, 397]'
13:03:44.926 DEBUG:    linesOfComments: '[1, 130, 266, 267, 268, 269, 142, 270, 271, 400, 273, 401, 20, 154, 284, 35, 38, 167, 295, 299, 44, 300, 301, 302, 175, 303, 311, 56, 315, 189, 62, 322, 195, 203, 335, 212, 217, 346, 95, 351, 227, 355, 228, 101, 114, 242, 244, 245, 246, 247, 120, 253]'

I couldn't find any error or warning in the log

From my understanding of the log:
sonar-cxx first parses header.h, where HEADER_IMPL is set
Then it parses header.hpp but the define doesn't seem to be set... unless I force it

@guwirth
Copy link
Collaborator

guwirth commented Apr 2, 2021

Hi @pcasenove,

What I understood so far:

  • Inside of header.h the file header.hpp should not be parsed because the include guard is set before the include.
  • The file header.hpp contains the implementation and should be parsed as source file.

How are your settings:

  • sonar.cxx.suffixes.sources
  • sonar.cxx.suffixes.headers

Regards

@pcasenove
Copy link
Author

Hi,
sonar.cxx.suffixes.headers: .hxx, .hpp, .hh, .h
sonar.cxx.suffixes.sources: .cxx,.cpp,.cc,.c

The hpp file is an inline implementation file. For gcc it is considered as a header.

@guwirth
Copy link
Collaborator

guwirth commented Apr 2, 2021

Hi,

the plugin iterates over all source code files and analyze one after the other. If a source code file includes a header file this is also analyzed.

In your case it seems that a cpp file calls header.h. The define HEADER_IMPL inside of header.h prevents the analysis of header.hpp (because of the include guard). Wondering anyway why the code is in the header file? To analyze header.hpp it must be included somewhere else. If not you should add .hpp to sonar.cxx.suffixes.sources.

With your compiler: How is header.hpp called?

Regards,

@kabasset
Copy link

kabasset commented Apr 2, 2021

Hi @guwirth

I'm working with @pcasenove on this, let me clarify.

We have a classical implementation of a template function (could be the same for any inline function) in a dedicated inline implementation file header.hpp. The function is declared in header.h and used in some main.cpp, like so:

// header.h

#ifndef HEADER
#define HEADER

template<typename T>
void f();

#define HEADER_IMPL // The macro is set before including header.hpp
#include "header.hpp"
#undef HEADER_IMPL

#endif

// header.hpp

#ifdef HEADER_IMPL // This is an ifdef, not an ifndef!

template<typename T>
void f() {
  // do something...
}

#endif

// main.cpp

#include "header.h"

int main(int argc, char *argv[]) {
  f<int>();
  return 0;
}

Therefore, main.cpp includes header.h which itself includes header.hpp. The macro is set in header.h and tested in header.hpp, so the contents of header.hpp is usable.
This construct aims at avoiding that users include directly header.hpp.

@guwirth
Copy link
Collaborator

guwirth commented Apr 2, 2021

Hi,

thx. I missed the ifdef thought it’s ifndef. So you think the define HEADER_IMPL in header.h is not forward propagated to header.hpp? Last question: What is missing in the SonarQube UI? Then I think have to try it out by myself... Maybe this #1489.

Regards,

@pcasenove
Copy link
Author

In SonarQube UI, the LOC is set to 0. It seems that no analysis is done at all on the hpp file also.
When we comment the ifdef, we have LOC correctly computed.

@guwirth guwirth added bug and removed question labels Apr 2, 2021
@guwirth guwirth added this to the 1.3.3 milestone Apr 2, 2021
@guwirth
Copy link
Collaborator

guwirth commented Apr 3, 2021

@pcasenove @wawane do you have the possibility to test cxx plugin 2.0. think the problem is solved with 2.0. You can download latest snapshot from here https://ci.appveyor.com/project/SonarOpenCommunity/sonar-cxx/branch/master/artifacts

@pcasenove
Copy link
Author

Hi
Is it compatible with SQ7.9 LTS? If so, I could give feedback by end of next week.
Otherwise which version should I install?

Thanks

@guwirth
Copy link
Collaborator

guwirth commented Apr 5, 2021

Hi @pcasenove,

we are testing cxx plugin 2.0 currently always with 7.9 LTS and latest 8.x (you see this in LOG files of Travis). End of April SonarSource is planning to release next LTS 8.9. So easiest would be to use 2.0. We didn't plan to do another 1.3 release. If no other possibility we have to verify if backport of bugfix is possible - but can take awhile. Focus is currently on next LTS.

https://github.com/SonarOpenCommunity/sonar-cxx/wiki/Upgrade-the-Plugin

Regards,

@pcasenove
Copy link
Author

Hi,
I've upgraded to v2 and adjusted the calls, but I encounter the same behavior:
11:35:43.287 DEBUG: process unit 'header.hpp'
11:35:43.289 DEBUG: 'Public API' metric for 'header.hpp': total=0, undocumented=0
And it goes to the next file

If I comment the ifdef the file gets analyzed and I have a log like:
11:35:43.287 DEBUG: process unit 'header.hpp'
11:36:04.747 DEBUG: 'Public API' metric for 'header.hpp': total=20, undocumented=1

No error or warning in the log file, only DEBUG messages

Pierre

@guwirth
Copy link
Collaborator

guwirth commented Apr 6, 2021

@pcasenove thanks for the feedback, will verify it this week.

@guwirth guwirth modified the milestones: 1.3.3, 2.0.0 Apr 6, 2021
guwirth added a commit to guwirth/sonar-cxx that referenced this issue Apr 8, 2021
@guwirth
Copy link
Collaborator

guwirth commented Apr 9, 2021

Hello @pcasenove,

I did a first test #2101 with your provided sample.

What you can see in the LOG file: all three files are indexed main.cpp, header.h and header.hpp.
The macros are propagated as expected. Each file is analyzed independently as a source code file:

  • process unit main.cpp -> include header.h -> include header.hpp. Parsing main.cpp "counts" only metrics from main.cpp and not from the includes. Counting also the #include content would be confusing in the UI (e.g. NCLOC).
  • process unit (as a source file) header.h -> include header.hpp. Because of #ifndef HEADER the source code is handled.
  • process unit (as a source file) include header.hpp. Because of #ifdef HEADER_IMPL the source code is not handled.

This explains also why you "see" the source code in case you set a global macro HEADER_IMPL. From the plugin point of view, everything works as expected. The question is how others do that with templates with the include guards?

Regards,

21:01:54.600 INFO: Indexing files...
21:01:54.615 DEBUG: 'src/main.cpp' indexed with language 'cxx'
21:01:54.616 DEBUG: 'src/header.h' indexed with language 'cxx'
21:01:54.618 DEBUG: 'src/header.hpp' indexed with language 'cxx'
21:01:54.619 INFO: 3 files indexed
...
21:01:56.100 INFO: Sensor CXX [cxx]
21:01:56.251 DEBUG: sonar.cxx.metric.api.file.suffixes: [.hxx, .hpp, .hh, .h]
21:01:56.260 DEBUG: 'Complex Functions' metric threshold (cyclomatic complexity): 10
21:01:56.261 DEBUG: 'Big Functions' metric threshold (LOC): 20
21:01:56.302 DEBUG: global include directories: []
21:01:56.316 DEBUG: global macros: [{__STDC__:1}, {__TIME__:"??:??:??"}, {__STDC_HOSTED__:1}, {__FILE__:"file"}, {__DATE__:"??? ?? ????"}, {__has_include:1}, {__cplusplus:201103L}, {__LINE__:1}]
21:01:56.316 DEBUG: process unit '/home/travis/build/SonarOpenCommunity/sonar-cxx/integration-tests/testdata/macro_propagation_project/src/main.cpp'
21:01:56.343 DEBUG: process unit '/home/travis/build/SonarOpenCommunity/sonar-cxx/integration-tests/testdata/macro_propagation_project/src/header.h'
21:01:56.360 DEBUG: 'Public API' metric for 'header.h': total=1, undocumented=1
21:01:56.361 DEBUG: process unit '/home/travis/build/SonarOpenCommunity/sonar-cxx/integration-tests/testdata/macro_propagation_project/src/header.hpp'
21:01:56.368 DEBUG: 'Public API' metric for 'header.hpp': total=0, undocumented=0
21:01:56.379 INFO: Load project repositories
21:01:56.737 DEBUG: GET 404 http://localhost:9000/batch/project.protobuf?key=macro_propagation_project | time=358ms
21:01:56.737 DEBUG: Project repository not available - continuing without it
21:01:56.737 INFO: Load project repositories (done) | time=358ms
21:01:56.738 DEBUG: 'src/main.cpp' generated metadata with charset 'UTF-8'
21:01:56.770 DEBUG: Not enough content in 'src/main.cpp' to have CPD blocks, it will not be part of the duplication detection
21:01:56.779 DEBUG: 'src/header.h' generated metadata with charset 'UTF-8'
21:01:56.781 DEBUG: Not enough content in 'src/header.h' to have CPD blocks, it will not be part of the duplication detection
21:01:56.782 DEBUG: 'src/header.hpp' generated metadata with charset 'UTF-8'
21:01:56.784 DEBUG: Not enough content in 'src/header.hpp' to have CPD blocks, it will not be part of the duplication detection
21:01:56.784 INFO: Sensor CXX [cxx] (done) | time=684ms

@guwirth
Copy link
Collaborator

guwirth commented Apr 9, 2021

@guwirth guwirth changed the title Template implementation issue templates: separate interface from implementation (.inl, .tcc) Apr 9, 2021
@kabasset
Copy link

kabasset commented Apr 9, 2021

Dear @guwirth

I would expect the extension to have no impact, but we can give a try to renaming the inline file to e.g. header.tcc.
As for the inverted guard, it's the only way I know to avoid that people include directly this file, but it might be a bit of an overkill. In any case, since this is correct C++ and correctly compiled, I guess sonar-cxx should be able to cope with such structure.

@pcasenove
Copy link
Author

Hi,
If we want to rename these files to .tcc, shall I add the tcc extension in sonar.cxx.suffixes.headers or in sonar.cxx.suffixes.sources ?

Pierre

@guwirth
Copy link
Collaborator

guwirth commented Apr 9, 2021

add the tcc extension in sonar.cxx.suffixes.headers or in sonar.cxx.suffixes.sources ?

@pcasenove with 1.3 in sonar.cxx.suffixes.sources / with 2.0 there is only one setting sonar.cxx.file.suffixes

@guwirth
Copy link
Collaborator

guwirth commented Apr 9, 2021

Hi @wawane,

expect the extension to have no impact, but we can give a try to renaming the inline file to e.g. header.tcc

You are right, that the cxx plugin doesn't behave different. The point is that users can see from the file extension that they should not include the file directly.

I had a look to GCC STL and Microsoft ATL/MFC library: Both don't add an inverted include guard. My question is still if this is really needed?

In any case, since this is correct C++ and correctly compiled, I guess sonar-cxx should be able to cope with such structure.

It also works exactly like in C++. If you compile header.hpp as source code file and HEADER_IMPL is not defined the preprocessor removes the code. That's exactly what you see in the SQ UI.

My first idea was also to simply ignore the preprocessor directives (#if #else #ifdef, ...) in a source code file. But this then leads to syntax errors in many other places. Imaging code like this:

#ifdef WIN32
void sample(uint32)
#else
void sample(uint64)
#endif
{
}

My poposals:

  1. Rename the file extensions to .tcc and remove the include guards => Users see from the naming convention that they should not include the file
  2. Or: Add a second macro which is always set parsing with cxx plugin e.g. __CXX__. Think GCC has already something like this called somehow LINT, PCLINT (search in the header files) ???
// header.h

#ifndef HEADER
#define HEADER

template<typename T>
void f();

#define HEADER_IMPL // The macro is set before including header.hpp
#include "header.hpp"
#undef HEADER_IMPL

#endif

// header.hpp

#if defined(HEADER_IMPL) || defined(__CXX__) // This is an ifdef, not an ifndef!

template<typename T>
void f() {
  // do something...
}

#endif

// main.cpp

#include "header.h"

int main(int argc, char *argv[]) {
  f<int>();
  return 0;
}

Other ideas are welcome!

Regards,

@pcasenove
Copy link
Author

pcasenove commented Apr 9, 2021

Hi,
I've tested number 2 (adding a macro) and it worked. I'll talk with @wawane on the best way to go.

I lost a bit of time (2 hours....) trying to make it work on the file on which sonar-cxx crashes... (but I saw opened bugs on this):

14:39:19.831 ERROR: Unable to parse file:ImageWrapper.h
14:39:19.832 ERROR: Parse error at line 49 column 45:

   43: std::type_info &readTypeid(fitsfile *fptr);
   44: 
   45: 
   46: 
   47: 
   48: template <long n = 2>
  -->  FitsIO::Position<n> readShape(fitsfile *fptr);
   50: 
   51: 
   52: 
   53: 
   54: template <typename T, long n = 2>
   55: void updateShape(fitsfile *fptr, const FitsIO::Position<n> &shape);
   56: 
   57: 
   58: 
   59: 
   60: template

As the parsing of the .h was failing, the hpp was not taken into account...

@guwirth
Copy link
Collaborator

guwirth commented Apr 10, 2021

Hi @pcasenove, template issue should be solved with #2102. please try it and give feedback.... Regards

@kabasset
Copy link

We've finally implemented #if defined(HEADER_IMPL) || defined(SONAR_CXX) and it gives the expected result. Yet, I still consider this as a workaround, because HEADER_IMPL is defined. If I understood correctly what you wrote @guwirth, each file is analyzed independently, and macros are not forwarded through includes, which is why HEADER_IMPL is not seen when parsing header.hpp alone. I guess that's one limitation of static analyzers in general?

@guwirth
Copy link
Collaborator

guwirth commented Apr 13, 2021

@wawane explanation: In order for SonarQube to display source code, it must be assigned to a programming language, which is done via the file extension. Then SonarQube works through one file after the other and assigns "metrics" (LOC, Line is Executable, ...).

To parse a source code file correctly, the cxx plugin must also read the header files to extract the macros from them. But the #include files are not treated as source code files in this step.

In this case:

main.cpp
   header.h // #define HEADER_IMPL
       header.hpp // #if defined(HEADER_IMPL)
       
header.hpp // #if defined(HEADER_IMPL)

The cxx plugin works as it should. Securing the .tcc files via include guards is unusual? It also seems to make little sense to use different include-guards for the .tcc-files, since they are only temporarily valid.

@pcasenove
Copy link
Author

Hi
@guwirth : I've just tested latest cxx v2 plugin (sorry for the delay) and I confirm that the template issue is fixed. No more crashes, while we have 2/3 files not analyzed with 1.3.3. Thanks a lot for the fix.

@guwirth guwirth removed this from the 2.0.0 milestone Apr 14, 2021
@guwirth
Copy link
Collaborator

guwirth commented Apr 14, 2021

I close this one. From cxx point of view everything is correct. Please reopen it if you have an idea how to improve plugin.

@guwirth guwirth closed this as completed Apr 14, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

No branches or pull requests

3 participants