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

#pragma once usage to prevent unwanted inclusions from happening even once #363

Closed
wants to merge 3 commits into from

Conversation

datadiode
Copy link
Contributor

@datadiode datadiode commented Aug 24, 2024

This implements the idea behind #159 (comment) in a generic way.
It works by allowing #pragma once to take a wildcard expression to sort out the unwanted include files.

Example from some project of mine, to prevent some inclusions which would otherwise cause cppcheck to abort prematurely:

#ifdef CPPCHECK
#   pragma once "boost/*"
#   pragma once "google/protobuf/*"
#   pragma once "*.pb.h"
#endif

Going with Cppcheck's --include option is particularly convenient for the purpose.

@datadiode datadiode force-pushed the pragmaOddOnce branch 5 times, most recently from fb3f18d to 414909d Compare August 30, 2024 19:53
@firewave
Copy link
Collaborator

firewave commented Sep 3, 2024

It is not possible to use C++11 since simplecpp still needs to support C++03.

Also using std::regex is highly discouraged because it is so slow that is essentially unusable in any production code.

@datadiode
Copy link
Contributor Author

datadiode commented Sep 3, 2024

Would SRELL be an acceptable replacement for std::regex?

@firewave
Copy link
Collaborator

firewave commented Sep 3, 2024

I guess it is sure to say that any third-party dependency is out of the question.

@datadiode
Copy link
Contributor Author

That seems plausible.
Then I'd rather go for a simplistic wildcard match, preferably by reusing the fnmatch() from https://compressionratings.com/d_archiver_template.html.

@datadiode datadiode force-pushed the pragmaOddOnce branch 3 times, most recently from f0c55c7 to f7fbdd1 Compare September 28, 2024 12:46
@Tal500
Copy link

Tal500 commented Sep 29, 2024

Would SRELL be an acceptable replacement for std::regex?

@datadiode @firewave FYI: this library is the state of the art solution for compile-time-baking of regexp engine in c++ (source: cppcon first presentation of Herb):
https://github.com/hanickadot/compile-time-regular-expressions

@danmar
Copy link
Owner

danmar commented Sep 29, 2024

FYI: this library is the state of the art solution for compile-time-baking of regexp engine in c++

Thanks for the suggestion.

I think CTRE is awesome!! but it requires modern compilers. and minimum c++17 as far as I know.

@@ -3292,6 +3292,40 @@ static std::string getTimeDefine(const struct tm *timep)
return std::string("\"").append(buf).append("\"");
}

// Reuse some code from https://compressionratings.com/d_archiver_template.html
// SPDX-License-Identifier: CC0-1.0
class simplecpp::Mask : public std::string {
Copy link
Owner

Choose a reason for hiding this comment

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

I feel this Mask class is not very elegant imho.

  • I don't feel it should inherit from std::string, it's not used as a std::string.
  • I dont see the point to read the i flag from the string in the constructor like that instead of passing a parameter. Will there be unintentional case matching if the string contains a random "i" somewhere or not.
  • the fnmatch shouldn't be a template function
  • I don't see why match a const char* instead of a std::string.. seems redundant to call .c_str()

// # pragma once "google/protobuf/*"
// # pragma once "*.pb.h"
// #endif
std::set<Mask> pragmaOddOnce;
Copy link
Owner

Choose a reason for hiding this comment

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

well it could be std::set<std::string> pragmaOnceMasks; .. instead of creating a Mask class what would be the downside to just provide a regular function like:

bool match(const std::string& mask, const std::string& value, bool i);

@danmar
Copy link
Owner

danmar commented Sep 29, 2024

I don't see that #159 talks about pragma once Ideally it would be better to preprocess the macros properly instead... but I don't know how difficult it would be to reach that point.

I reopened the #159 I think I was too quick to close it before.

I have never seen this pragma once mask syntax was it something you invented? I would rather not add this if you invented this syntax.

@datadiode
Copy link
Contributor Author

datadiode commented Oct 2, 2024

The invented extension to the #pragma once behavior intends to avoid a need to follow the last-resort idea of hardcoding the exclusion of boost headers like so, as mentioned in said comment:

+        if (header.rfind("boost/", 0) == 0)
+            continue;
+

#pragma isn't standardized. I don't think it is too wrong to overload it with some esoteric feature which is meaningless to other preprocessors. #pragma ignore_include may feel less wrong though.

@danmar
Copy link
Owner

danmar commented Oct 3, 2024

to follow the last-resort idea of hardcoding the exclusion of boost headers like so, as mentioned in said comment:

That hardcoding does feel worse. However this simplecpp hack is not the ideal solution.

When I read #159 it does seem that we have many problems but missing defines is a major one. That is not a simplecpp bug. It should be the responsibility of the tool (i.e. cppcheck) to provide proper defines. We have some cppcheck ticket(s) about teaching cppcheck defines better.

If cppcheck command --library=boost is used can that somehow prevent that the boost headers are included? It could remove the boost include path(s) maybe. Using --library=boost is something we recommend already for cppcheck users.

@datadiode
Copy link
Contributor Author

I did think of proposing them a command line option to tell cppcheck to ignore any include paths it picks up from a project file if they match some regular expression. Will go ahead with that idea.

@datadiode datadiode closed this Oct 4, 2024
@firewave
Copy link
Collaborator

firewave commented Oct 4, 2024

I did think of proposing them a command line option to tell cppcheck to ignore any include paths it picks up from a project file if they match some regular expression. Will go ahead with that idea.

That already exists. It is the -i option.

@datadiode
Copy link
Contributor Author

I did think of proposing them a command line option to tell cppcheck to ignore any include paths it picks up from a project file if they match some regular expression. Will go ahead with that idea.

That already exists. It is the -i option.

According to the usage information, -i applies only to source files so header files included by source files are not matched.

@firewave
Copy link
Collaborator

According to the usage information, -i applies only to source files so header files included by source files are not matched.

You are of course right. I might have been thinking of something else (and maybe something I needed at some point).

It might also play into https://trac.cppcheck.net/ticket/12359 and https://trac.cppcheck.net/ticket/13204.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants