FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

ValueFlow: pass `ErrorLogger` by reference into `ValueFlow::setValues()` / removed need for `LifetimeStore::Context` by firewave · Pull Request #5299 · cppcheck-opensource/cppcheck · GitHub

/ cppcheck Public

ValueFlow: pass ErrorLogger by reference into ValueFlow::setValues() / removed need for LifetimeStore::Context - #5299

Merged
firewave merged 9 commits into
cppcheck-opensource:mainfrom
firewave:ptr-ref-v
Apr 4, 2024
ConversationCommits (9)ChecksFiles changed
Merged

ValueFlow: pass ErrorLogger by reference into ValueFlow::setValues() / removed need for LifetimeStore::Context#5299
firewave merged 9 commits into
cppcheck-opensource:mainfrom
firewave:ptr-ref-v

Conversation

firewave commented Aug 7, 2023

Copy link
Copy Markdown
Collaborator

No description provided.

danmar approved these changes Aug 7, 2023
View reviewed changes

danmar left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

I am fine with that.

firewave marked this pull request as draft August 7, 2023 18:39

firewave commented Aug 7, 2023

Copy link
Copy Markdown
Collaborator Author

Needs some reworking around givenACodeSampleToTokenize first.

firewave mentioned this pull request Aug 8, 2023
Draft
firewave mentioned this pull request Mar 5, 2024
Merged
firewave force-pushed the ptr-ref-v branch 4 times, most recently from e7bc1ff to ed3cb49 Compare March 9, 2024 16:42
firewave changed the title ValueFlow: pass ErrorLogger by reference into ValueFlow::setValues() ValueFlow: pass ErrorLogger by reference into ValueFlow::setValues() / removed need for LifetimeStore::Context Mar 9, 2024
firewave force-pushed the ptr-ref-v branch 4 times, most recently from d0d4904 to 38c8092 Compare March 9, 2024 23:15
pfultz2 reviewed Mar 10, 2024
View reviewed changes
Comment thread lib/forwardanalyzer.cpp
ValuePtr<Analyzer> analyzer;
const TokenList& tokenList;
ErrorLogger* const errorLogger;
ErrorLogger& errorLogger;

pfultz2 Mar 10, 2024

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.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

This makes the class not assignable. This should be a pointer.

firewave Mar 11, 2024

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.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

That ship has already sailed - some one line above.

I will take a look at adding that clang-tidy check about this (there was a case of false positives which I am not sure is fixed yet) and there's also #4785 and some discussions which related to that (which apparently are not linked).

firewave Mar 11, 2024

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.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

FYI There also would be a compiler error if we would actually try to assign those types. I ran into this issue while working on it. So it is not a silent failure.

firewave Apr 2, 2024

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.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Also I encountered the first issue we have with this in some other changes. I will try to figure out how to properly handle it so it can be easily be applied in the future. Nothing to do here though since this doesn't change anything as we were using references before.

Comment thread lib/valueflow.cpp Outdated
mContext->errorLogger = errorLogger;
mContext->settings = &settings;
}
mutable Token* forwardTok{};

pfultz2 Mar 10, 2024

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.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

I don't know about using the mutable keyword here.

firewave Mar 11, 2024

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.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Me neither - but removing the const from the function had quite a ripple effect before - will check again. I think this might be a sensible use of mutable.

firewave Apr 2, 2024

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.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

The ripple effect wasn't as bad as I remember. The object was only const when passing it around so that was not fully enforced and is actually fine.

firewave force-pushed the ptr-ref-v branch 4 times, most recently from 111292c to 53dd484 Compare March 11, 2024 10:08
firewave commented Mar 11, 2024
View reviewed changes
Comment thread lib/tokenize.cpp
const bool doValueFlow = !disableValueflowEnv || (std::strcmp(disableValueflowEnv, "1") != 0);

if (doValueFlow) {
assert(mErrorLogger);

firewave Mar 11, 2024

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.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

After the change in ImportProject this can no longer happen. I did not change it to a reference yet since it used in a lot of redundant test code. As I plan to clean up that redundancy I will address it with that upcoming change instead of touching the code twice.

firewave commented Mar 11, 2024
View reviewed changes
Comment thread lib/tokenize.cpp
return true;
}

// cppcheck-suppress unusedFunction - used in tests only

firewave Mar 11, 2024

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.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

This was surprising but it actually foreshadows the cleanup I am planning to make.

firewave requested a review from danmar March 11, 2024 10:19
firewave force-pushed the ptr-ref-v branch 2 times, most recently from 8eac401 to 39369b4 Compare March 11, 2024 15:02
firewave marked this pull request as ready for review March 12, 2024 00:15
firewave merged commit 228f9cd into cppcheck-opensource:main Apr 4, 2024
firewave deleted the ptr-ref-v branch April 4, 2024 13:39
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

pfultz2 pfultz2 left review comments
danmar Awaiting requested review from danmar

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

Back | FazBrowse Home | New Git URL