GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — ValueFlow: pass `ErrorLogger` by reference into `ValueFlow::setValues()` / removed need for `LifetimeStore::Context` by firewave · Pull Request #5299 · cppcheck-opensource/cppcheck

Conversation

@firewave

@firewave firewave commented Aug 7, 2023

Copy link
Copy Markdown
Collaborator

No description provided.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
danmar
danmar approved these changes Aug 7, 2023
View reviewed changes

@danmar danmar left a comment

Copy link
Copy Markdown
Collaborator

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

I am fine with that.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@firewave
firewave marked this pull request as draft August 7, 2023 18:39
@firewave

firewave commented Aug 7, 2023

Copy link
Copy Markdown
Collaborator Author

Needs some reworking around givenACodeSampleToTokenize first.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@firewave firewave mentioned this pull request Aug 8, 2023
added (optional) lazy execution of ValueFlow #4521
Draft
@firewave
firewave force-pushed the ptr-ref-v branch from 1d0788a to 0f152ba Compare August 15, 2023 11:46
@firewave firewave mentioned this pull request Mar 5, 2024
testrunner: refactored givenACodeSampleToTokenize into SimpleTokenizer and SimpleTokenList / several related cleanups #6084
Merged
@firewave
firewave force-pushed the ptr-ref-v branch 4 times, most recently from e7bc1ff to ed3cb49 Compare March 9, 2024 16:42
@firewave 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
firewave force-pushed the ptr-ref-v branch 4 times, most recently from d0d4904 to 38c8092 Compare March 9, 2024 23:15
pfultz2
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 pfultz2 Mar 10, 2024

Copy link
Copy Markdown
Collaborator

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@firewave firewave Mar 11, 2024

Copy link
Copy Markdown
Collaborator Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

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).

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@firewave firewave Mar 11, 2024

Copy link
Copy Markdown
Collaborator Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@firewave firewave Apr 2, 2024

Copy link
Copy Markdown
Collaborator Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread lib/valueflow.cpp Outdated
mContext->errorLogger = errorLogger;
mContext->settings = &settings;
}
mutable Token* forwardTok{};

@pfultz2 pfultz2 Mar 10, 2024

Copy link
Copy Markdown
Collaborator

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@firewave firewave Mar 11, 2024

Copy link
Copy Markdown
Collaborator Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@firewave firewave Apr 2, 2024

Copy link
Copy Markdown
Collaborator Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@firewave
firewave force-pushed the ptr-ref-v branch 4 times, most recently from 111292c to 53dd484 Compare March 11, 2024 10:08
firewave
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 firewave Mar 11, 2024

Copy link
Copy Markdown
Collaborator Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@firewave
firewave force-pushed the ptr-ref-v branch from 53dd484 to 174b6fa Compare March 11, 2024 10:18
firewave
firewave commented Mar 11, 2024
View reviewed changes
Comment thread lib/tokenize.cpp
return true;
}

// cppcheck-suppress unusedFunction - used in tests only

@firewave firewave Mar 11, 2024

Copy link
Copy Markdown
Collaborator Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@firewave
firewave requested a review from danmar March 11, 2024 10:19
@firewave
firewave force-pushed the ptr-ref-v branch 2 times, most recently from 8eac401 to 39369b4 Compare March 11, 2024 15:02
@firewave
firewave marked this pull request as ready for review March 12, 2024 00:15
firewave added 3 commits April 2, 2024 21:29
@firewave
ValueFlow: pass ErrorLogger by reference into ValueFlow::setValues()
d6e12b8
@firewave
avoid some unchecked pointer dereferences
4595196
@firewave
testrunner: adjusted several Tokenizer creations
e27d171
firewave added 5 commits April 2, 2024 21:30
@firewave
valueflow.cpp: removed need for LifetimeStore::Context
eef052c
@firewave
Tokenizer: made ErrorLogger parameter mandatory in constructor
76a459a
@firewave
importproject.cpp: use TokenList instead of Tokenizer in `ItemDef…
c8de69b 
…initionGroup::conditionIsTrue()`
@firewave
tokenize.cpp: mitigated unusedFunction selfcheck warning
92cbc6c
@firewave
valueflow.cpp: mitigated naming-privateMemberVariable selfcheck war…
127d361 
…ning
@firewave
firewave force-pushed the ptr-ref-v branch from 39369b4 to 127d361 Compare April 2, 2024 19:30
@firewave
valueflow.cpp: removed mutable from lifetimeStore::forwardTok
589d6f3
@firewave
firewave merged commit 228f9cd into cppcheck-opensource:main Apr 4, 2024
@firewave
firewave deleted the ptr-ref-v branch April 4, 2024 13:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@pfultz2 pfultz2 pfultz2 left review comments
@danmar danmar Awaiting requested review from danmar

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

3 participants

@firewave @danmar @pfultz2
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.