Skip to content
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.

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

add feature detection macro MP_HAS (2) #262

Merged
merged 3 commits into from
Sep 2, 2019
Merged

Conversation

minad
Copy link
Member

@minad minad commented May 14, 2019

See #214. I wanted to try again if I can make this work without using variadic macros, this time only relying on C89.

@karel-m Could you please try again your compiler suite?

@sjaeckel @czurnieden This PR is simpler and better than the one before. Hopefully it also works on older compilers. This helps to clean up control flow. Another advantage is that the compiler will check all branches. Dead code elimination will make sure that the dead branches are not compiled.

@minad minad force-pushed the feature-detection2 branch 4 times, most recently from 933a50c to c91cb35 Compare May 14, 2019 19:32
@minad minad changed the title add feature detection macros MP_DEFINED and MP_HAS (2) add feature detection macro MP_HAS (2) May 14, 2019
@minad minad requested review from sjaeckel and czurnieden May 14, 2019 20:14
@minad minad mentioned this pull request May 16, 2019
@minad minad requested a review from karel-m May 18, 2019 21:46
@minad
Copy link
Member Author

minad commented May 19, 2019

@sjaeckel What do we have to check here to make sure that this change does not break any things? I don't think it does, but what do you guys need to be convinced 😉

If you are worried about dead code elim, try this with gcc -o dead dead.c

/* dead.c */
#include <stdio.h>

void does_not_exist(void);

int main(void) {
        if (sizeof("") == 1) {
                puts("We're ok");
        }
        if (sizeof("X") == 1) {
                does_not_exist();
        }
        return 0;
}

@minad
Copy link
Member Author

minad commented May 26, 2019

In contrast to the solution in #214, this works on MSVC 2015 and 2017 as I just tested using appveyor.

@sjaeckel
Copy link
Member

@karel-m can you please have a look if this works for your cases as well?

@minad
Copy link
Member Author

minad commented Jun 5, 2019

Which is exactly what I would call the slightly growing lack of attention to unusual compilers.

This is what I call "a typo", nothing to complain about. Thank you for the fix.

@karel-m
Copy link
Member

karel-m commented Jun 5, 2019

This is what I call "a typo", nothing to complain about.

Obviously. What I wanted to say is that nobody tried any compiler that hits #else branch.

@minad
Copy link
Member Author

minad commented Jun 6, 2019

@sjaeckel Ping. See also https://www.gnu.org/prep/standards/standards.html#Conditional-Compilation where c conditionals are recommended for conditional compilation.

@minad minad force-pushed the feature-detection2 branch 2 times, most recently from 6987dba to 2a9148e Compare June 12, 2019 10:02
@minad minad force-pushed the feature-detection2 branch 2 times, most recently from e51542e to efbabc1 Compare June 12, 2019 18:32
@minad minad removed the request for review from karel-m July 4, 2019 08:58
@minad
Copy link
Member Author

minad commented Jul 4, 2019

@sjaeckel What is your opinion about this PR now?

@sjaeckel
Copy link
Member

Obviously. What I wanted to say is that nobody tried any compiler that hits #else branch.

I'm not sure where the culprit has to be searched there... in the compiler providers who mostly try to mimic gcc or the ones who don't mimic gcc not being available easily...

Which compilers did we test this now with?

  • gcc down to 3.4
  • clang multiple versions
  • MSVC multiple versions
  • IBM xlc 11.1

Something missing? If this works with all those compilers I think it's safe to merge.

@minad
Copy link
Member Author

minad commented Jul 17, 2019

@sjaeckel yes, the compilers you listed are the ones we tested so far.

@minad
Copy link
Member Author

minad commented Jul 24, 2019

@sjaeckel I rebased this PR again on develop. CI all green.

@minad
Copy link
Member Author

minad commented Aug 1, 2019

Ping @sjaeckel. Could you merge this PR or are there things to do?

Copy link
Member

@sjaeckel sjaeckel left a comment

Choose a reason for hiding this comment

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

I still don't really like those complex if-elseif statements/chain e.g. in mp_mul() but I could live with it...

@czurnieden anything to add?

@czurnieden
Copy link
Contributor

@czurnieden anything to add?

To mp_mul?
Yes, but not today, that'll need a bit longer ;-)

Otherwise: no problems, can tick it off, too.

@minad
Copy link
Member Author

minad commented Aug 9, 2019

@czurnieden Thanks!

@sjaeckel

I still don't really like those complex if-elseif statements/chain e.g. in mp_mul() but I could live with it...

Yes, it might be possible to improve this. In order to not introduce any unwanted changes, I took the most direct translation from the intermingled #if and if branches.

@minad
Copy link
Member Author

minad commented Aug 12, 2019

@sjaeckel ping?

@sjaeckel sjaeckel merged commit a439ddf into develop Sep 2, 2019
@fperrad fperrad mentioned this pull request Sep 2, 2019
@minad minad deleted the feature-detection2 branch November 14, 2019 14:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants