SFTtech/openage

Replace likely macro with C++20 attribute

heinezen opened this issue · 9 comments

We currently use gcc's __builtin_expect() macro aliased as unlikely and likely for static branch prediction. Primarily, this optimizes checks for parser errors. This optimizes performance for code paths which are only taken under very specific circumstances.

Example:

if (unlikely(not this->value.exists())) {
    throw InternalError{"fetched nonexisting value of member"};
}

Since C++20, the standard supports attributes that do the same thing. Additionally, they are compiler agnostic, so they should also work in clang or msvc. They should also be more readable since they don't directly wrap the condition.

Basically the code gets changed to this:

if (not this->value.exists()) [[unlikely]] {
    throw InternalError{"fetched nonexisting value of member"};
}

If you want to discover the codebase or really like mundane tasks, then this is an issue for you :)


Also take note of the same issue in the nyan repository: SFTtech/nyan#105

I'd like to work on it.

@tarunsamanta2k20 Sweet :) Tell us if you need any directions.

@tarunsamanta2k20 Are you still working on this? I would like to work on it if you are not doing it anymore.

@heinezen should i replace all occurrences of likely and unlikely both?

@derekfrogget if you want you may do it for 'likely' only. I am doing this only for unlikely cases.

@tarunsamanta2k20 @derekfrogget both likely and unlikel can be replaced.

There's also the same issue for the nyan repo SFTtech/nyan#105 (i think you also wanted to do this @tarunsamanta2k20 , right?)

now it's only likely thats still there

@tarunsamanta2k20 @derekfrogget both likely and unlikel can be replaced.

There's also the same issue for the nyan repo SFTtech/nyan#105 (i think you also wanted to do this @tarunsamanta2k20 , right?)

@derekfrogget (he) will do for likely. Ping him. He is also interested to work on it.

Thanks everybody, now only the nyan repo task is left SFTtech/nyan#105 :)