Page MenuHomePhabricator

[clangd] Fix hover crashing on integral or enumeral casts
Needs ReviewPublic

Authored by gkll on May 26 2022, 1:24 PM.

Details

Reviewers
sammccall
Summary

When pretty printing the value of an expression, we cannot infer from
the type of the expression the type of the constant that the expression
evaluates to, as the expression might contain a type cast.

Diff Detail

Event Timeline

gkll created this revision.May 26 2022, 1:24 PM
Herald added a project: Restricted Project. · View Herald TranscriptMay 26 2022, 1:24 PM
gkll requested review of this revision.May 26 2022, 1:24 PM
nridge added a subscriber: nridge.May 28 2022, 8:25 PM

Oh wow, my mental model of these was all wrong.

Thank you! Do you want me to land this for you?

sammccall accepted this revision.May 31 2022, 11:40 AM
This revision is now accepted and ready to land.May 31 2022, 11:40 AM
gkll added a comment.May 31 2022, 12:21 PM

Oh wow, my mental model of these was all wrong.

Thank you! Do you want me to land this for you?

Yeah, that would be great, thank you!

Oh wow, my mental model of these was all wrong.

Thank you! Do you want me to land this for you?

Yeah, that would be great, thank you!

Sorry, can you provide an email address for attribution?

(Normally I can pick this up with arc patch, but not this time apparently)

gkll added a comment.May 31 2022, 12:56 PM

Oh wow, my mental model of these was all wrong.

Thank you! Do you want me to land this for you?

Yeah, that would be great, thank you!

Sorry, can you provide an email address for attribution?

(Normally I can pick this up with arc patch, but not this time apparently)

Maybe because I didn't upload the patch with the arc tooling.
You can attribute the patch to: gkll@mailbox.org

Landed as ce5ebf0b9113df8ccaec1bcfd6804fb16cdef69d but I had to revert due to test failures on windows:

Unfortunately neither of these give a stack trace, so I'm not yet sure what's wrong, beyond "some Optional is null when it shouldn't be".
A wild guess: long is shorter than a pointer on 64-bit windows, so that code doesn't even parse.
Indeed (int)&global_var doesn't parse for me on a 64-bit linux machine (32-bit int, 64-bit pointer).
The mystery in that case, though, is why the test output doesn't show parse error before crashing (TestTU is supposed to dump this unless you set ErrorOK).

It's late, but I'll try fixing and re-landing tomorrow.

gkll updated this revision to Diff 433174.May 31 2022, 1:47 PM

Changed return type of non-returning function in test code to void.

gkll updated this revision to Diff 433211.May 31 2022, 3:44 PM

Use uintptr_t instead of unsigned long for the casts, to ensure the pointer always fits into the integer type.
For example on Windows x64 that was previously not the case, because there unsigned long is only 32-bits wide.
Thus the pointer got truncated during the cast, which resulted in getHover() returning None instead of &global_var.

gkll updated this revision to Diff 433217.May 31 2022, 3:56 PM

Fixed formatting of Argv array.

Neat, looks like the pre-merge windows buildbot at least is happy!
I'll land this tomorrow.

clang-tools-extra/clangd/unittests/HoverTests.cpp
3221

As an alternative (just to avoid adding this option), I think adding "-target=x86_64-pc-linux-gnu" would force intptr_t to be long.
Up to you.

clang-tools-extra/clangd/unittests/TestTU.h
63

If you do add this option, please invert the sense, i.e. bool PredefineMacros = false;

(The double negative of Omit = false is a bit confusing to read)

And please expand the comment slightly: "... such as __INTPTR_TYPE__"

gkll updated this revision to Diff 433304.Jun 1 2022, 12:34 AM

Avoid double negation.

gkll marked 2 inline comments as done.Jun 1 2022, 12:56 AM

Thank you, and sorry for the chaos, this is my first submission to llvm :D

clang-tools-extra/clangd/unittests/HoverTests.cpp
3221

I prefer using the predefine.

gkll requested review of this revision.Aug 30 2022, 2:21 AM
gkll marked an inline comment as done.

Mhm, I think the change here has gone under the radar 🙈

Sorry about this! Landing now