Skip to content

fixed #13129 - apply --std with --clang / fixed usage of Standards::stdValue - #6742

Merged
firewave merged 1 commit into
cppcheck-opensource:mainfrom
firewave:stdvalue
Sep 29, 2024
Merged

fixed #13129 - apply --std with --clang / fixed usage of Standards::stdValue#6742
firewave merged 1 commit into
cppcheck-opensource:mainfrom
firewave:stdvalue

Conversation

@firewave

Copy link
Copy Markdown
Collaborator

No description provided.

@firewave

This comment was marked as outdated.

@firewave

firewave commented Sep 1, 2024

Copy link
Copy Markdown
Collaborator Author

"fun" fact: I found part of this in a 2.5 year old local changelist...

@firewave
firewave force-pushed the stdvalue branch 3 times, most recently from b9240dc to 2b1e751 Compare September 2, 2024 09:48
@firewave
firewave force-pushed the stdvalue branch 5 times, most recently from 4149fc7 to e8590cd Compare September 25, 2024 08:44
@firewave firewave changed the title fixed usage of Standards::stdValue fixed #13129 - apply --std with --clang / fixed usage of Standards::stdValue Sep 25, 2024
@firewave
firewave marked this pull request as ready for review September 25, 2024 08:48
@firewave

Copy link
Copy Markdown
Collaborator Author

CC @ludviggunne please also take a look at what you made me do 😉

@ludviggunne

Copy link
Copy Markdown
Collaborator

Sorry!

@firewave

Copy link
Copy Markdown
Collaborator Author

Sorry!

I was asking for a review not pleasantries. 😁

@ludviggunne

Copy link
Copy Markdown
Collaborator

Looks good to me.

@firewave

Copy link
Copy Markdown
Collaborator Author

Thanks!

Comment thread test/fixture.h Outdated
Comment thread lib/standards.h
std::string stdValueCPP;

bool setC(std::string str) {
stdValue = str;

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

I just wonder if setC and setCPP are needed.

If only setStd is called from outside we could implement all the logic in that directly or make setC and setCPP private? I feel maybe the code would be a bit simpler but on the other hand testability is important too.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

That should be detected by the CI then. Maybe there is a conflict. Will take a look later. And more changes are coming and will integrate that cleanup if possible.

@danmar danmar left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

I see nothing wrong here.. so feel free to merge. I don't have a strong opinion about setC/setCPP, feel free to make that decision.

@firewave
firewave merged commit 3b36db9 into cppcheck-opensource:main Sep 29, 2024
@firewave
firewave deleted the stdvalue branch September 29, 2024 13:18
ludviggunne pushed a commit to ludviggunne/cppcheck that referenced this pull request Oct 19, 2024
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