This is an archive of the discontinued LLVM Phabricator instance.

[clangd] Fix bugs in main-file include patching for stale preambles

Authored by kadircet on Feb 2 2023, 9:03 AM.


  • Make sure main file includes are present even when they're not patched (because they didn't change or we're explicitly not patching them).
  • Populate extra fields for includes, which can be used by include-cleaner.

Diff Detail

Event Timeline

kadircet created this revision.Feb 2 2023, 9:03 AM
Herald added a project: Restricted Project. · View Herald TranscriptFeb 2 2023, 9:03 AM
Herald added a subscriber: arphaman. · View Herald Transcript
kadircet requested review of this revision.Feb 2 2023, 9:03 AM
kadircet updated this revision to Diff 494347.Feb 2 2023, 9:53 AM
  • Also populate additional fields
nridge added a subscriber: nridge.Feb 4 2023, 7:17 PM

Code looks reasonable, but I don't understand why the changes are being made - can you explain/link to a bug in the commit message?


This doesn't seem related to the patch description, can you update it?


why the null check?


seems a bit more future-proof to copy the whole thing and just overwrite the fields you want to preserve. (also would avoid the need for the comment)


this just echoes the code - say why instead?

(would also be good to have the reason in the commit message)

kadircet updated this revision to Diff 495790.Feb 8 2023, 3:24 AM
kadircet marked 4 inline comments as done.
kadircet retitled this revision from [clangd] Patch includes even without any changes to [clangd] Fix bugs in main-file include patching for stale preambles.
kadircet edited the summary of this revision. (Show Details)
  • Address comments & update commit message

as discussed offline, we might have includes coming from scanned contents whose are not part of the preamble, e.g. because they're in a disabled PP region.

sammccall accepted this revision.Feb 8 2023, 7:28 AM
sammccall added inline comments.

if It->second is null, then all the #includes of this header from the baseline preamble were in disabled sections, so it's *very* likely this one is too.

I think we're better not pushing onto PP.PreambleIncludes at all in this case, rather than pushing an unresolved one - this is how the MainFileIncludes looks like when an #include is in a disabled section and there's no patching happening.



This revision is now accepted and ready to land.Feb 8 2023, 7:28 AM
kadircet updated this revision to Diff 495884.Feb 8 2023, 10:45 AM
kadircet marked 2 inline comments as done.
  • use rawstrings in test
kadircet added inline comments.Feb 8 2023, 10:55 AM

agreed. as discussed sent out D143597