reduced size of ValueFlow::Value

[firewave]

No description provided.

[firewave]

@chrchr-github how high/low can this go? would a 8-bit value be enough for this?

[chrchr-github]

8bit unsigned should plenty, unless negative values are used somewhere?

[firewave]

It looks like it can go negative based on the code so it should be signed.

[firewave]

This is only used when --debug is specified. So we waste 16 bytes for having a full type instead of a pointer.

I did hack this out locally and it saves about 3% IR and several operations no longer show up at all in profiling it appears we just narrowly exceed some threshold for some compiler optimization(s) or allocation pattern.

[firewave]

Still need to clean up the macros. And I would like to get the TODOs addressed as well.

The constant for the padding also needs to be adjusted so takes the default alignment into account and we do not overpad. I was also thinking about a macro.

[pfultz2]

What the reason for this?

[firewave]

That is explicit padding.

[danmar]

explicit padding.. wait.. sounds like you are suboptimizing for a particular compiler now. Does this make the code faster on all compilers that is used to cppcheck? Will it make the code faster in future also.

[firewave]

It gets rid of excessive padding and the explicit padding makes sure the warning gets triggered when this is modified in the future.

It will speed up things because less memory needs to be allocated (we did something similar with Token and TokenImpl).

But you are right that this adheres to the alignment to a specific platform (which is 8 bytes in this case). I do not think it should have that much impact on other platforms as only the explicit padding should affect them and that is kept quite low with 28 bits (which is way less than what we save).

[firewave]

This is mostly clean now and reduces the size from 168 bytes to 136 bytes.

But the debugpath code is broken. And I still want to cleanup the redundancies in the config.h macros.

[firewave]

This is mostly clean now and reduces the size from 168 bytes to 136 bytes.

But the debugpath code is broken. And I still want to cleanup the redundancies in the config.h macros.

I backed out the debugpath changes for now. The size is now only reduced from 168 bytes to 152 bytes.

[danmar]

I backed out the debugpath changes for now. The size is now only reduced from 168 bytes to 152 bytes.

Does it make sense to hide debugpath behind some build flag? If somebody needs it then he can build cppcheck with that. Or maybe leave it out in the release builds?

[danmar]

An old wisdom is that it is a good idea to order members by size.
See i.e.: http://katecpp.github.io/struct-members-order/
I have seen good results in a embedded project.
I guess it's pretty platform-dependent and would make no difference on some platforms.

would it be a good idea to use "pragma packed" on this struct?

[firewave]

An old wisdom is that it is a good idea to order members by size.

Interesting. I have not done much in this memory so far because there were always still bigger issues in the projects I worked on.

would it be a good idea to use "pragma packed" on this struct?

Given the low amount of explicit padding we have to apply (and my limited understanding of packed) that might not be necessary. I also think that might come only into play is when you really want to optimize for cachelines (which I have no experience working with) and I assume this will always be out-of-scope for this application.

[firewave]

Does it make sense to hide debugpath behind some build flag? If somebody needs it then he can build cppcheck with that. Or maybe leave it out in the release builds?

That is included in the --debug output so changing that will potentially open a can of worms.

[danmar]

I am fine with these changes.

[pfultz2]

In the future, I would like to introduce dynamic attributes as some of these attributes are only used for some rare occasions, and it doesnt make sense to always have a static field for such attributes:

struct Attributes {

    template<class A>
    const auto& get() const
    {
        return read<A>().get();
    }

    template<class A, class T>
    void set(T x) const
    {
        return read<A>().set(x);
    }

    template<class A>
    const A& read() const
    {
        return static_cast<const A*>(attr.at(key<A>()).get());
    }

    template<class A>
    A& write()
    {
        return static_cast<A*>(attr.at(key<A>()).get());
    }

private:
    std::unordered_map<std::type_index, ValuePtr<Attribute>> attrs;

    template<class A>
    static std::type_index key()
    {
        return std::type_index(typeid(A));
    }
};

We could probably make this copy-on-write as well to make the copies cheaper.

[firewave]

In the future, I would like to introduce dynamic attributes as some of these attributes are only used for some rare occasions, and it doesnt make sense to always have a static field for such attributes:

That would be something to consider. If I find the time (haha) I will give it a look.