This is an archive of the discontinued LLVM Phabricator instance.

[clangd] #undef macros inside preamble patch
ClosedPublic

Authored by kadircet on Feb 1 2023, 10:34 AM.

Details

Summary

That way we can stop generating false macro redefinition diagnostics.

Depends on D142890

Diff Detail

Event Timeline

kadircet created this revision.Feb 1 2023, 10:34 AM
Herald added a project: Restricted Project. · View Herald TranscriptFeb 1 2023, 10:34 AM
Herald added a subscriber: arphaman. · View Herald Transcript
kadircet requested review of this revision.Feb 1 2023, 10:34 AM

I can't understand from the description, code, or testcases what problem this is fixing. Can you clarify, ideally by improving the testcases?

It seems to introduce a false negative in the case that the preamble *does* contain a definition of an already-defined macro, which probably needs to be called out.

I can't understand from the description, code, or testcases what problem this is fixing. Can you clarify, ideally by improving the testcases?

Yeah should've elaborated on this one. As explained offline the issue is macro token contents are not preserved in the PCH and are re-lexed when needed (for diagnosing macro-redefined errors in our case). Since PCH contents are stale, when we retrieve token contents based on ranges stored in the PCH using the new file contents, we get a false-positive.
This patch tries to avoid such false positive diagnostics by emitting an #undef statement before any macro definition we're going to emit.


It seems to introduce a false negative in the case that the preamble *does* contain a definition of an already-defined macro, which probably needs to be called out.

I am failing to follow this one. I can think of two different cases:

#define FOO 1
#define FOO 2
;

changes to

#define BAR
#define FOO 1
#define FOO 2
;

we won't have diagnostics here, not because of the #undef logic, but because we're not moving the diagnostic ranges around.

Other case is we've

#define FOO 1
;
#define FOO 2

and it gets changed to

#define BAR
#define FOO 1
;
#define FOO 2

now the issue is we're emitting re-definition diag to a location inside the preamble patch, but we don't translate the location back to main-file. again it has nothing to do with the #undef logic proposed here (and fixed by D143095).

nridge added a subscriber: nridge.Feb 4 2023, 7:15 PM
kadircet added inline comments.Feb 6 2023, 9:46 AM
clang-tools-extra/clangd/Preamble.cpp
734

i know we've discussed emitting all the #undef directives at the top of the patch together rather than right before #define directives. but i am failing to remember any benefits and looping twice here doesn't feel great. so leaving it as-is for now, LMK if it feels wrong.

kadircet updated this revision to Diff 495185.Feb 6 2023, 9:46 AM
  • Update tests
sammccall accepted this revision.Feb 8 2023, 12:59 AM
sammccall added inline comments.
clang-tools-extra/clangd/Preamble.cpp
216

initialize to pp_unknown

clang-tools-extra/clangd/unittests/PreambleTests.cpp
701–702

as elsewhere, I think this writing this as a single raw string without reusing BaselinePreamble is easier to follow

707–712

named ranges

This revision is now accepted and ready to land.Feb 8 2023, 12:59 AM
kadircet updated this revision to Diff 495891.Feb 8 2023, 10:57 AM
kadircet marked 3 inline comments as done.
  • Use raw strings
  • Rebase
This revision was automatically updated to reflect the committed changes.