Skip to content

ClangImport: improved location parsing / better handling for "prologue" TypedefDecls#7115

Merged
firewave merged 6 commits intocppcheck-opensource:mainfrom
firewave:clangimport-loc
Dec 20, 2024
Merged

ClangImport: improved location parsing / better handling for "prologue" TypedefDecls#7115
firewave merged 6 commits intocppcheck-opensource:mainfrom
firewave:clangimport-loc

Conversation

@firewave
Copy link
Copy Markdown
Collaborator

No description provided.

@firewave firewave marked this pull request as draft December 17, 2024 19:03
@firewave
Copy link
Copy Markdown
Collaborator Author

@danmar @chrchr-github
In the future it would be great if could also add the source which generated the AST as a comment. This would make it easier to understand what it represents. Also the AST is constantly evolving so it might be possible we will no longer be able to reproduce the same output with newer versions.

This also makes it harder to convert the tests to a different AST format (i.e. -ast-dump=json / https://trac.cppcheck.net/ticket/13398).

@firewave
Copy link
Copy Markdown
Collaborator Author

I just wanted to get rid of the setLang() hack so I could hopefully proceed with other related refactorings but as usual there was ripple effect which required more changes (for the better IMO).

@firewave firewave marked this pull request as ready for review December 17, 2024 19:28
@chrchr-github
Copy link
Copy Markdown
Collaborator

What is the status of clangimport anyway? The one time I checked, it didn't seem to work very well.

@firewave
Copy link
Copy Markdown
Collaborator Author

What is the status of clangimport anyway? The one time I checked, it didn't seem to work very well.

Still experimental and that basically reflects the reality of the implementation. There was talk of dropping it completely at some point.

@danmar
Copy link
Copy Markdown
Collaborator

danmar commented Dec 20, 2024

What is the status of clangimport anyway? The one time I checked, it didn't seem to work very well.

Still experimental and that basically reflects the reality of the implementation. There was talk of dropping it completely at some point.

In my opinion it's still experimental and it should be rewritten to use the json output from clang instead. I have no plans to do it anytime soon though.

I think I might have suggested that this could be dropped but I don't think so anymore.

We have discussed this feature in Cppcheck Solutions company at some point and it's a bit interesting for us. Maybe someday we will decide that it's needed and ensure it's implemented.

@danmar
Copy link
Copy Markdown
Collaborator

danmar commented Dec 20, 2024

Also the AST is constantly evolving so it might be possible we will no longer be able to reproduce the same output with newer versions.

yeah I fear that our clangimport gets more and more broken for every clang version. :-(

@firewave firewave merged commit 04e0f68 into cppcheck-opensource:main Dec 20, 2024
@firewave firewave deleted the clangimport-loc branch December 20, 2024 19:20
@firewave
Copy link
Copy Markdown
Collaborator Author

yeah I fear that our clangimport gets more and more broken for every clang version. :-(

Not essentially. The test coverage keeps improving slightly and there's the (in the CI still disabled) TEST_CPPCHECK_INJECT_CLANG which will easily shake out the most obvious issues. Something to look into if all known problems have been fixed.

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.

3 participants