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

c++ apply integer conversions add tests #284

Conversation

i-garrison
Copy link
Contributor

This change fixes a few places where integer conversion was missing, and adds a few tests using corrected evaluation results.

I do use these primarily for constexpr evaluations in CDT UI so there are probably more cases where integer conversions are missing (e.g. I did not looked at C code at all.)

@i-garrison i-garrison changed the title Pr/c++ apply integer conversions add tests c++ apply integer conversions add tests Feb 12, 2023
@github-actions
Copy link

github-actions bot commented Feb 13, 2023

Test Results

   540 files     540 suites   14m 38s ⏱️
9 645 tests 9 623 ✔️ 22 💤 0
9 656 runs  9 634 ✔️ 22 💤 0

Results for commit 79a9018.

♻️ This comment has been updated with latest results.

@i-garrison i-garrison marked this pull request as draft February 13, 2023 07:00
Currently type of parameters of instantiated template function is ignored while
preparing activation record, which makes constexpr evaluation of instantiated
body use types of arguments in function call expression instead:

    template<typename T> bool f(T t) { return t > 0; }
    t<unsigned int>(-1); // CDT returns false because conversion is not done

Fix this by applying EvalTypeId to argument if cost of standard conversions is
Rank.CONVERSION to make sure createActivationRecord() would populate activation
record with argument values matching template parameter types.
@i-garrison i-garrison force-pushed the pr/c++-apply-integer-conversions-add-tests branch from 4d45445 to 79a9018 Compare February 13, 2023 07:43
@i-garrison i-garrison marked this pull request as ready for review February 13, 2023 07:45
Copy link
Member

@jonahgraham jonahgraham left a comment

Choose a reason for hiding this comment

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

Thanks for this move in the right direction.

[...] where integer conversions are missing (e.g. I did not looked at C code at all.)

It is ok by me that these other cases aren't done. I am glad you are able to move your use case forward.

PS I am quite curious about your use case as this code hasn't gotten any love for a while it is interesting to me to know who and why someone is investing time in it.

@jonahgraham jonahgraham merged commit dc8313b into eclipse-cdt:main Feb 13, 2023
@i-garrison
Copy link
Contributor Author

Thanks @jonahgraham Well there is nothing major behind this at the moment - I'm just using CDT to explore various code bases (both C and C++) for ages now, some of those projects recently moved to C++20, cdt.cloud is not ready yet and I cannot find anything about ongoing LSP integration efforts - looks like that stalled. So I decided to add a few missing features to CDT which would in short term unblock browsing c++20 code to some extent, and part of that is correct constexpr evaluations.

LSP integration looks more future-proof but I do not see anyone doing that for CDT at the moment.

@i-garrison i-garrison deleted the pr/c++-apply-integer-conversions-add-tests branch February 13, 2023 18:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants