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

added (optional) lazy execution of ValueFlow #4521

Draft

firewave wants to merge 4 commits into danmar:main from firewave:vf

Collaborator

firewave commented Sep 29, 2022

No description provided.

firewave force-pushed the vf branch 3 times, most recently from 2b93752 to 07ba92f Compare

September 29, 2022 13:26

pfultz2 reviewed

View reviewed changes

lib/tokenlist.h

+                  std::function<void()> mValuesFunc;
+                  mutable bool mValuesSet;
+                  mutable bool mInSetValues;

Contributor

pfultz2 Oct 4, 2022

These two variables can be moved into the mValuesFunc.

Contributor

pfultz2 Oct 4, 2022 •

edited

Loading

We could do something similar to what we do for memoize:

template<class F>
static std::function<void()> once(F f)
{
    bool ran = false;
    return [=]() mutable {
        if (ran)
            return;
        ran = true;
        f();
    };
}

Collaborator Author

firewave Oct 4, 2022

This was very hacky. I just tried to get rid what I wanted it to do.

These two variables can be moved into the mValuesFunc.

Good point.

We could do something similar to what we do for [memoize]

Will take a look. I know there are patterns for this but I rarely use them so I keep forgetting them.

pfultz2 reviewed

View reviewed changes

lib/token.cpp

               }
               // cppcheck-suppress unusedFunction
               const ValueFlow::Value* Token::getContainerSizeValue(const MathLib::bigint val) const
               {
-                  if (!mImpl->mValues)
+                  if (values().empty())

Contributor

pfultz2 Oct 4, 2022

values().empty() is redundant here.

Collaborator Author

firewave Oct 4, 2022 •

edited

Loading

I kept it to avoid the function calls (which obviously won't do much) since some of these might be called loooooots of times. I haven't profiled any of this yet.

Collaborator Author

firewave Aug 8, 2023

I would not call it "redundant" but "unnecessary". Still needs to be profiled.

pfultz2 reviewed

View reviewed changes

lib/token.cpp

               const ValueFlow::Value* Token::getMovedValue() const
               {
-                  if (!mImpl->mValues)
+                  if (values().empty())

Contributor

pfultz2 Oct 4, 2022

values().empty() is redundant here.

pfultz2 reviewed

View reviewed changes

lib/token.cpp

               }
               const ValueFlow::Value* Token::getMaxValue(bool condition, MathLib::bigint path) const
               {
-                  if (!mImpl->mValues)
+                  if (values().empty())

Contributor

pfultz2 Oct 4, 2022

values().empty() is redundant here.

pfultz2 reviewed

View reviewed changes

lib/token.cpp

               }
               const ValueFlow::Value* Token::getValue(const MathLib::bigint val) const
               {
-                  if (!mImpl->mValues)
+                  if (values().empty())

Contributor

pfultz2 Oct 4, 2022

values().empty() is redundant here.

pfultz2 reviewed

View reviewed changes

lib/token.cpp

               {
                   if (tok->exprId() == 0)
                       return false;
-                  return mImpl->mValues &&
-                         std::any_of(mImpl->mValues->begin(), mImpl->mValues->end(), [&](const ValueFlow::Value& value) {
+                  return !values().empty() &&

Contributor

pfultz2 Oct 4, 2022

values().empty() is redundant here.

pfultz2 reviewed

View reviewed changes

lib/token.cpp

    
                      return value.isKnown() && value.isIntValue();

                  });

              }

              bool Token::hasKnownValue() const

              {

                  return mImpl->mValues && std::any_of(mImpl->mValues->begin(), mImpl->mValues->end(), std::mem_fn(&ValueFlow::Value::isKnown));

                  return !values().empty() && std::any_of(values().begin(), values().end(), std::mem_fn(&ValueFlow::Value::isKnown));

Contributor

pfultz2 Oct 4, 2022

values().empty() is redundant here.

pfultz2 reviewed

View reviewed changes

lib/token.cpp

@@ @@ -2316,22 +2314,21 @@ std::shared_ptr<ScopeInfo2> Token::scopeInfo() const @@
               bool Token::hasKnownIntValue() const
               {
-                  if (!mImpl->mValues)
+                  if (values().empty())

Contributor

pfultz2 Oct 4, 2022

values().empty() is redundant here.

pfultz2 reviewed

View reviewed changes

lib/token.cpp

               const Token *Token::getValueTokenMaxStrLength() const
               {
-                  if (!mImpl->mValues)
+                  if (values().empty())

Contributor

pfultz2 Oct 4, 2022

values().empty() is redundant here.

pfultz2 reviewed

View reviewed changes

lib/token.cpp

               const Token *Token::getValueTokenMinStrSize(const Settings *settings, MathLib::bigint* path) const
               {
-                  if (!mImpl->mValues)
+                  if (values().empty())

Contributor

pfultz2 Oct 4, 2022

values().empty() is redundant here.

pfultz2 reviewed

View reviewed changes

lib/token.cpp

                       return !v.isImpossible() && v.isIntValue() && v.intvalue >= val;
                   });
               }
               const ValueFlow::Value * Token::getInvalidValue(const Token *ftok, nonneg int argnr, const Settings *settings) const
               {
-                  if (!mImpl->mValues || !settings)
+                  if (values().empty() || !settings)

Contributor

pfultz2 Oct 4, 2022

values().empty() is redundant here.

pfultz2 reviewed

View reviewed changes

lib/token.cpp

                       return !v.isImpossible() && v.isIntValue() && v.intvalue <= val;
                   });
               }
               const ValueFlow::Value * Token::getValueGE(const MathLib::bigint val, const Settings *settings) const
               {
-                  if (!mImpl->mValues)
+                  if (values().empty())

Contributor

pfultz2 Oct 4, 2022

values().empty() is redundant here.

Contributor

pfultz2 commented Oct 4, 2022

I think its a lot cleaner to use values() instead of mImpl->mValues. However, !mImp->mValues is not the equivalent of !values.empty(). If we use values() then the checks for null can be removed.

Collaborator Author

firewave commented Oct 4, 2022

I think its a lot cleaner to use values() instead of mImpl->mValues.

That was my takeaway as well after I "cracked" this and saw only one method needs to access the raw pointers - which is quite nice. I will extract the encapsulation and cleanup parts of this into a separate PR but it still needs some work (see below).

However, !mImp->mValues is not the equivalent of !values.empty(). If we use values() then the checks for null can be removed.

I am aware of that. I already fixed up a few mistakes I made in earlier revisions.

But it is still very much WIP...

I still have to profile this with valueflow enabled and disabled.

Also the lazy execution doesn't save anything at all.
Only when used in conjunction with #4362 it will actually do something since there's currently no way to control specific checks and even the special ones don't do that as evident by the above PR and several tickets I filed. This is a bigger thing and quite a can of worms but I have some ideas on how to incrementally address this to enhance the control and get rid of my hacks used for profiling and the CI.
I was hoping that maybe some checks can be adjusted to "delay" usage of the vaueflow data so we could profit in more cases but without proper control about what check is run it won't do much in the end.

There's also some tests failing with the lazy execution. But I guess they just lack the lazy execution hook. Still it is interesting and maybe we should make valueflow more explicit in tests so we know what depends on it. It might also help with determining how to increase the test coverage. But I haven't looked into this at all.

firewave added 4 commits

October 5, 2022 11:11


          Token: avoid unnecessary direct access of TokenImpl::mValues

dfa2fb3


          moved TokensFrontBack from token.h to tokenlist.h

c4f1685


          tokenize.cpp: small refactoring

63e12eb


          added lazy execution of ValueFlow via DISABLE_VALUEFLOW=2 environme…

58dcddb

…nt variable

firewave force-pushed the vf branch from 07ba92f to 58dcddb Compare

October 5, 2022 09:19

firewave mentioned this pull request

Add a --check-level option #4942

Merged

Collaborator Author

firewave commented Aug 8, 2023

This should "help" (as in executing less code) with tests which use the Tokenizer but do not rely on the ValueFlow. That might also fix the issue I hit in #5299.

firewave mentioned this pull request

avoid duplicated workflow invocations for pushes in PRs made from root repository branches #5377

Merged

firewave mentioned this pull request

moved TokensFrontBack from token.h to tokenlist.h #5547

Merged

firewave mentioned this pull request

enabled and mitigated debug warnings in cfg tests #5840

Merged

firewave mentioned this pull request

Add --check-level=fast option that reuses much faster valueflow analysis from 1.90 #6097

Draft

Collaborator Author

firewave commented Apr 20, 2024

I will try to pull out the wrapper function for values so this can finally progress.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet