This is an archive of the discontinued LLVM Phabricator instance.

[clangd] Introduce config compilation for External blocks
ClosedPublic

Authored by kadircet on Nov 4 2020, 2:19 AM.

Details

Summary

Compilation logic for External blocks. A few of the high level points:

  • Requires exactly one-of File/Server at a time:
    • Server is ignored in case of both, with a warning.
    • Having none is an error, would render ExternalBlock void.
  • Ensures mountpoint is an absolute path:
    • Interprets it as relative to FragmentDirectory.
    • Defaults to FragmentDirectory when empty.
  • Marks Background as Skip.

Depends on D90748.

Diff Detail

Event Timeline

kadircet created this revision.Nov 4 2020, 2:19 AM
Herald added a project: Restricted Project. · View Herald TranscriptNov 4 2020, 2:19 AM
kadircet requested review of this revision.Nov 4 2020, 2:19 AM
sammccall added inline comments.Nov 6 2020, 8:29 AM
clang-tools-extra/clangd/ConfigCompile.cpp
282

My math friends would call this "case bashing" :-)
In particular I'm cringing at the thought of adding a third option here (it's not likely, but I think the code would be easier to understand if it was clear that two is an arbitrary number).

What about:

unsigned SourceCount = External.File.hasValue() + External.Server.hasValue();
if (SourceCount != 1)
  return diag(Error, "Exactly one of File and Server must be set");

(I don't think separate error messages for missing/multiple, or any particular behavior for multiple, is particularly useful)

284

these missing ranges are potentially pretty difficult for users - they won't know e.g. which file the error occurred in.

Do you think we can Located<> the whole ExternalBlock?

298

somewhere we should be converting the paths to native slashes, I think.

311

pull out a method like Optional<string> makeAbsolute(StringRef Path, StringRef Description)?

311

would also be good to include "Because this configuration is not associated with a directory"

328

we do all this validation and wind up writing back into the same data structure that can't represent it.
We don't actually have to validate again later, because now our invariants are rules, not suggestions. Still, it seems fragile, and violates a maxim that got stuck in my head: https://lexi-lambda.github.io/blog/2019/11/05/parse-don-t-validate/

Can we have an type for this spec in config, and have only a single slot for it?
It could be as simple as:

struct ExternalIndexSpec { 
  enum {File, Remote} Kind;
  std::string Location; // specific to kind, hack this up later if need be
  std::string MountPoint;
};

I think the single biggest improvement is that we'd model "replacing the index for this config" as a single assignment rather than 5 coordinated ones. So if you want something less different, putting the Optional around the config struct instead of individual fields would be fine too. (though that would require it to be named)

333

hang on, you say "under the mountpoint", but... that's not what's happening.

If we want that behavior, we need to use SourceInfo::Directory as the mountpoint (and so probably give users a way to set that in the global config fragment).

kadircet marked 5 inline comments as done.Nov 9 2020, 12:23 PM
kadircet added inline comments.
clang-tools-extra/clangd/ConfigCompile.cpp
284

I was doing that initially, but they felt similarly annoying, as they point at value corresponding to External block rather than the whole ,value range. In addition to that it only covers the first line :/

Happy to do that if you think that's still better than an empty/invalid range.

333

hang on, you say "under the mountpoint", but... that's not what's happening.

Umm, why? We bail-out at the beginning of apply if filepath doesn't start with mountpoint ?

kadircet updated this revision to Diff 303956.Nov 9 2020, 12:23 PM
  • Define an ExternalIndexSpec structure, and populate that during compile.
sammccall accepted this revision.Nov 9 2020, 1:18 PM
sammccall added inline comments.
clang-tools-extra/clangd/ConfigCompile.cpp
278

consistent with other helpers, can we avoid passing errors around, and instead issue the diagnostic here and return optional?

Signature seems overly general: the Path argument is always a Located<string>, the Base is always the fragment directory. These seem likely to always hold.

284

I think it's better than the empty range, even if it just gives us a location for the block, that tells us which file and which fragment.

(sorry, not sure what ",value range" is...)

291

why not path::make_absolute?

318

comment echoes the code, which is not hard to read - remove?

333

Sorry, I was confused...

This revision is now accepted and ready to land.Nov 9 2020, 1:18 PM
kadircet updated this revision to Diff 304064.Nov 10 2020, 12:03 AM
kadircet marked 7 inline comments as done.
  • Use ExternalBlock's range for diags.
  • Move diagnostic emitting to makeAbsolute.
This revision was landed with ongoing or failed builds.Nov 22 2020, 12:13 PM
This revision was automatically updated to reflect the committed changes.
thakis added a subscriber: thakis.Nov 22 2020, 4:30 PM

This breaks tests on Windows: http://45.33.8.238/win/28408/step_9.txt

Please take a look, and revert for now if it takes a while to fix.