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

refs #10692 - added command-line option --cpp-header-probe to probe headers and extension-less files for Emacs C++ marker #6324

Merged
merged 2 commits into from
May 23, 2024
Merged

refs #10692 - added command-line option --cpp-header-probe to probe headers and extension-less files for Emacs C++ marker #6324

merged 2 commits into from
May 23, 2024

Conversation

firewave
Copy link
Collaborator

No description provided.

@firewave
Copy link
Collaborator Author

Still work in progress...

The standard library and LLVM are specifying this among others. Just running it on /usr/include will encounter lots of actual usages.

This will be disabled by default but I am considering enabling it by default in the release afterwards.

Will also add a commit which adds it to our headers. Will most likely do it in a separate PR.

@firewave
Copy link
Collaborator Author

Requires #6325 to be merged first.

After #6211 has landed (if even possible) and we finally upgraded to PCRE2 (see https://trac.cppcheck.net/ticket/10610) we should replace most of the messy parsing code with a regular expression.

@chrchr-github
Copy link
Collaborator

What is the canonical use case for this?

@firewave firewave mentioned this pull request Apr 22, 2024
Merged
@firewave
Copy link
Collaborator Author

What is the canonical use case for this?

That is described in the ticket. It is mainly for IDE integrations. When you open a header by default it will be treated as C file and when it is actually C++ code it will error out. So you need to specify --language=c++ so the analysis is performed. But that would also force (and override the existing Path::identify() calls) all source files to be treated as C++ even though they might be C code.

For the CLI usage this should not have any impact at all since the probing would only be performed if you explicitly pass a file with the *.h or no extension (like the C++ standard headers). I probably should change the command-line option to --cpp-header-probe (or similar) indicate that.

This is based on the implementation of the CLion plugin which only has a single global configuration for the options being provided.

On a side note it already helped to detect two issues hard to spot issues. 😀

@firewave firewave mentioned this pull request Apr 22, 2024
Merged
@chrchr-github
Copy link
Collaborator

What is the canonical use case for this?

That is described in the ticket. [...]

Thanks for the explanation. IDEs were mentioned in the ticket, but I didn't make the connection to IDE integration.

@firewave
Copy link
Collaborator Author

firewave commented Apr 22, 2024 •
edited

Thanks for the explanation. IDEs were mentioned in the ticket, but I didn't make the connection to IDE integration.

I just realized there is no upstream ticket for this. It is mentioned in the known issues though: https://github.com/johnthagen/clion-cppcheck?tab=readme-ov-file#analyzing-header-files.

@firewave firewave changed the title fixed #10692 - added command-line option --cpp-probe to probe headers and extension-less files for Emacs marker fixed #10692 - added command-line option --cpp-header-probe to probe headers and extension-less files for Emacs marker Apr 22, 2024
@firewave
Copy link
Collaborator Author

I also encountered some performance issues with some headers even when I disable ValueFlow and all checks so it is probably related to the tokenizing or preprocessing. I have not looked into that yet.

@firewave
Copy link
Collaborator Author

I also encountered some performance issues with some headers even when I disable ValueFlow and all checks so it is probably related to the tokenizing or preprocessing. I have not looked into that yet.

Turns out this only occurs in a debug build so there is no issue.

@firewave firewave changed the title fixed #10692 - added command-line option --cpp-header-probe to probe headers and extension-less files for Emacs marker fixed #10692 - added command-line option --cpp-header-probe to probe headers and extension-less files for Emacs C++ marker Apr 22, 2024
@firewave firewave force-pushed the cpp-probe branch 7 times, most recently from 36708cf to 78807ce Compare April 24, 2024 08:47
@firewave
Copy link
Collaborator Author

firewave commented Apr 24, 2024 •
edited

To test this with existing headers I patched out some code for speed, enabled the logging in hasEmacsCppMarker() and ran find /usr/include -type f | xargs -n 1 bin/cppcheck -D__GNUC__ --cpp-header-probe -q 2> /dev/null (FYI that found 2812 markers).

It seems this uncovered bad markers in the LLVM headers which I reported upstream: llvm/llvm-project#89965.

@firewave firewave changed the title fixed #10692 - added command-line option --cpp-header-probe to probe headers and extension-less files for Emacs C++ marker refs #10692 - added command-line option --cpp-header-probe to probe headers and extension-less files for Emacs C++ marker Apr 24, 2024
@firewave
Copy link
Collaborator Author

This does not currently handle markers in /* */ comments. I will add support for that in a follow-up PR.

@firewave firewave force-pushed the cpp-probe branch 6 times, most recently from 799b1aa to 56f257d Compare April 25, 2024 21:08
@firewave firewave force-pushed the cpp-probe branch 4 times, most recently from 0439266 to 3cc9090 Compare April 26, 2024 09:22
firewave
firewave commented Apr 26, 2024
View reviewed changes
lib/tokenize.cpp Outdated
@@ -8052,7 +8052,8 @@ void Tokenizer::unmatchedToken(const Token *tok) const
void Tokenizer::syntaxErrorC(const Token *tok, const std::string &what) const
{
printDebugOutput(0);
throw InternalError(tok, "Code '"+what+"' is invalid C code. Use --std or --language to configure the language.", InternalError::SYNTAX);
// TODO
throw InternalError(tok, "Code '"+what+"' is invalid C code.\nUse --std or --language to configure the language or --cpp-header-probe to detect C++ headers via the Emacs marker.", InternalError::SYNTAX);
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This obviously needs to be reworded but I couldn't come up with something better yet.

I also moved the references to the options to the verbose part of the message. That should be done for a lot more messages but will be tackled at a future date.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I already re-worded it before I made it ready for review.

@firewave firewave marked this pull request as ready for review May 7, 2024 11:53
danmar
danmar reviewed May 21, 2024
View reviewed changes
lib/path.cpp Show resolved Hide resolved
danmar
danmar reviewed May 22, 2024
View reviewed changes
lib/settings.h Show resolved Hide resolved
firewave
firewave commented May 23, 2024
View reviewed changes
lib/path.cpp
findAndReplace(marker, "Mode:", "");
marker = trim(marker);
if (marker == "C++" || marker == "c++") {
// NOLINTNEXTLINE(readability-simplify-boolean-expr) - TODO: FP
Copy link
Collaborator Author

@firewave firewave May 23, 2024 •
edited

Choose a reason for hiding this comment

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

Upstream issue: llvm/llvm-project#93157.

@firewave firewave merged commit 2263d30 into danmar:main May 23, 2024
63 checks passed
@firewave firewave deleted the cpp-probe branch May 23, 2024 18:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@danmar danmar danmar approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
3 participants
@firewave @chrchr-github @danmar