This is an archive of the discontinued LLVM Phabricator instance.

[clangd] Don't rely on builtin headers for document-link.test.
ClosedPublic

Authored by hokein on Jan 28 2021, 11:55 PM.

Details

Summary

This test seems to be failing at HEAD.

Diff Detail

Event Timeline

hokein created this revision.Jan 28 2021, 11:55 PM
hokein requested review of this revision.Jan 28 2021, 11:55 PM
Herald added a project: Restricted Project. · View Herald TranscriptJan 28 2021, 11:55 PM
kadircet accepted this revision.Jan 31 2021, 11:30 PM
kadircet added a subscriber: sammccall.

thanks, lgtm! but I would wait for a while to see if someone(that remembers why this test was specifically asserting for built-in headers) will object.

clang-tools-extra/clangd/test/document-link.test
1–4

nit: drop empty line

2

s/create/Create/

3

this is clever! I believe there was a reason for this test to be asserting built-in headers (I can't seem to remember, maybe @sammccall does), but asserting through a mock header should also be fine, I suppose.

one thing though, we need to clean up %t, e.g. rm -rf %t.

(and regrading the failure at head, resource_dir path has changed with release cut from something/12.0.0 to something/13.0.0 so it should go away once you build new clang via ninja clang)

This revision is now accepted and ready to land.Jan 31 2021, 11:30 PM
sammccall accepted this revision.Feb 1 2021, 6:29 AM

Thanks for fixing this, we don't want to depend on clang and I didn't realize we still did!

clang-tools-extra/clangd/test/document-link.test
3

this is clever!

Indeed, I almost wish we had a less-clever way to do this but I can't think of one :-)

I believe there was a reason for this test to be asserting built-in headers (I can't seem to remember, maybe @sammccall does), but asserting through a mock header should also be fine, I suppose.

Nah, I think it was just to avoid complex setup (and builtin over stdlib as it's a somewhat less crazy dep).

hokein updated this revision to Diff 320454.Feb 1 2021, 6:42 AM

address comments.

This revision was landed with ongoing or failed builds.Feb 1 2021, 6:45 AM
This revision was automatically updated to reflect the committed changes.
hokein marked 3 inline comments as done.