[PATCH] D82736: [clangd] Rename FS.view(None) to FS.viewWithDefaultCWD()

[Arthur O'Dwyer via Phabricator via cfe-commits]

Quuxplusone abandoned this revision.
Quuxplusone added a comment.

Superseded by D82793 . :)


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D82736/new/

https://reviews.llvm.org/D82736



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D82736: [clangd] Rename FS.view(None) to FS.viewWithDefaultCWD()

[Sam McCall via Phabricator via cfe-commits]

sammccall added a comment.

In D82736#2120089 , @Quuxplusone wrote:

> In D82736#2119262 , @sammccall wrote:
>
> > `viewWithDefaultCWD` suggests to me the default has some semantics which 
> > don't exist, if using this API "shape" I'd substitute `Arbitrary` here
>
>
> I'm naturally 100% fine with that. I can continue updating this patch if you 
> like; otherwise my default will be to abandon this patch on the assumption 
> that anything I can do, you can do better. :)D82793 
> 


Thanks! I put together D82793 . Feel free to 
ignore the below as my "for the record" nitpicking,..

>> - I think the argument for changing the *public* API is particularly weak 
>> (or missing?)...
> 
> Well, see, I would call that "the two overloads do **completely different 
> things**," right? One changes the CWD and the other doesn't.

Not really, that's an implementation detail! The contract is request vs no 
request of working directory.
With both overloads virtual (current state), the most reasonable implementation 
for a truly virtual FS is probably for one to return `new FSImpl(Path)` and the 
other to call `new FSImpl("/")`. But both the strategy and the directory chosen 
are impl-dependent.

> Also, splitting a single function `view(Optional)` into a pair of 
> overloads `view(PathRef)` and `view(NoneType)` does not work the way you seem 
> to be expecting it to.

It's a bit rude to assume that if you can't think of a reason, it must be 
everyone else that's ignorant. To recap what's been previously stated:

- an overload set is required to allow passing both `string` and `NoneType`, 
because string doesn't implicitly convert to Optional.
- large overload sets are prone to ambiguity problems
- we have out-of-tree callers that use yet-different string types
- the change that introduced this interface was primarily a renaming patch that 
touched many files (and so each change is trivial, but work to revert)
- making the overload set support actual optionals touches few files, but may 
introduce subtle compile problems and need to be reverted
- therefore we deferred the `Optional` overload until the dust settled 
from that patch (it hasn't) and possibly until it's actually needed.

I'm sure there are different ways to do this, but I can assure you that both 
author and reviewer understand how function calls work!


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D82736/new/

https://reviews.llvm.org/D82736



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D82736: [clangd] Rename FS.view(None) to FS.viewWithDefaultCWD()

[Arthur O'Dwyer via Phabricator via cfe-commits]

Quuxplusone added a comment.

In D82736#2119262 , @sammccall wrote:

> `viewWithDefaultCWD` suggests to me the default has some semantics which 
> don't exist, if using this API "shape" I'd substitute `Arbitrary` here


I'm naturally 100% fine with that. I can continue updating this patch if you 
like; otherwise my default will be to abandon this patch on the assumption that 
anything I can do, you can do better. :)



> - I think the argument for changing the *public* API is particularly weak (or 
> missing?)...

and Kadir wrote:

> The two overloads in the base class literally do the **same** thing ... It 
> was originally meant to be a single function with signature 
> view(llvm::Optional) but this result in wrapping the likely 
> std::string parameter in an explicit llvm::StringRef constructor on almost 
> all callsites, as an optional can't be constructed implicitly. 
> Hence we rather chose to split the parameter type into two overloads.

Well, see, I would call that "the two overloads do **completely different 
things**," right? One changes the CWD and the other doesn't. So they don't do 
"the same thing" and therefore shouldn't be part of the same overload set. 
Overload sets are specifically used in statically polymorphic code, where you 
might have a piece of generic code, like a template, that always wants to do 
"the same thing" but on different argument types. That's never the case here. 
Also, splitting a single function `view(Optional)` into a pair of 
overloads `view(PathRef)` and `view(NoneType)` does not work the way you seem 
to be expecting it to. If you actually //had// an `Optional p`, then 
calling `view(p)` simply wouldn't compile. Optionals don't "automagically 
visit" like that (and `llvm::Optional`'s monadic `map` method doesn't work 
quite like that either).
The one thing your overload set would theoretically let you do is, with a 
`std::variant`,

  std::variant v;
  auto FSP = std::visit([](auto x){ return TFS.view(x); }, v);  // call either 
view(PathRef) or view(NoneType) depending on the variant's current runtime state

But the current code doesn't ever do anything like that. (Nor should it. It 
would be confusing.)  Since there is no //physical// reason (e.g. visitor 
pattern) for the two functions to have the same name, and there is no 
//logical// reason (e.g. "doing-the-same-thing-ness") for them to have the same 
name, therefore in my book they should have different names.

> - I could certainly live with `private: virtual T viewImpl() = 0`. Sounds 
> like some will find that nicer, it's not very intrusive, and we don't need to 
> maintain a flag-divergence from clang

👍 IIUC, you're proposing to keep the overload set, but make the whole overload 
set public-and-nonvirtual, and have `view(NoneType)` dispatch to `viewImpl()`. 
I still think the overload set is misguided for the reasons given earlier in 
this reply, but I agree that a public nonvirtual interface backed by a single 
virtual implementation method would be better stylistically than what's there 
now, and it would also make GCC happy.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D82736/new/

https://reviews.llvm.org/D82736



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D82736: [clangd] Rename FS.view(None) to FS.viewWithDefaultCWD()

[Sam McCall via Phabricator via cfe-commits]

sammccall added a comment.

Yeah, IMO we spent a bunch of time on these names, I still like them, and I 
don't find the arguments since then convincing. New info is of course the 
warning, and the lack of consensus to change it at clang level.

As for this proposal specifically

- `viewWithDefaultCWD` suggests to me the default has some semantics which 
don't exist, if using this API "shape" I'd substitute `Arbitrary` here
- I think the argument for changing the *public* API is particularly weak (or 
missing?). It's more important than the private API, and no change is required 
to silence the warning. A new name is inevitably a mouthful, and it ties our 
hands for adding the `Optional` overload.
- I could certainly live with `private: virtual T viewImpl() = 0`. Sounds like 
some will find that nicer, it's not very intrusive, and we don't need to 
maintain a flag-divergence from clang

> [virtual]... not that we need it today or it is certain that we'll need one 
> someday

We have an out-of-tree implementation that should be using this, though may not 
have switched yet. But this also certainly will never really matter, so let's 
drop `virtual` for simplicity.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D82736/new/

https://reviews.llvm.org/D82736



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D82736: [clangd] Rename FS.view(None) to FS.viewWithDefaultCWD()

[Kadir Cetinkaya via Phabricator via cfe-commits]

kadircet added a comment.

Thanks, renaming was also another option we had in mind, see 
https://reviews.llvm.org/D81920#2109901 and possibly the following comments. I 
thought it was discussed in the disable-the-warning thread, but to elaborate a 
little more:

Naming is hard in general, this case is no different.

The two overloads in the base class literally do the **same** thing, one of 
them doesn't **change** the CWD. Hence the `Default` doesn't reflect what it 
returns, it's likely to be in an arbitrary state. There are some parts of the 
code that always make use of absolute paths, and it is meaningless for them to 
have sense of "CWD". Since we can't always create a view with some **sane** 
CWD, we needed such an option.

It was originally meant to be a single function with signature 
`view(llvm::Optional)` but this result in wrapping the likely 
`std::string` parameter in an explicit `llvm::StringRef` constructor on almost 
all callsites, as an optional can't be constructed implicitly. Hence 
we rather chose to split the parameter type into two overloads.

So IMHO, having two functions with different names doesn't reflect the intent 
as clearly as code's current state (e.g. via overloads). This is definitely 
subjective though and depends on the taste of people that's reading/maintaining 
the code in question.

As for dropping the virtual in the latter function, it was done to support a 
pure virtual implementation of a ThreadsafeFS whom stored the CWD internally, 
not that we need it today or it is certain that we'll need one someday. So it 
can be changed when the need arises.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D82736/new/

https://reviews.llvm.org/D82736



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D82736: [clangd] Rename FS.view(None) to FS.viewWithDefaultCWD()

[Arthur O'Dwyer via Phabricator via cfe-commits]

Quuxplusone created this revision.
Quuxplusone added reviewers: sammccall, kadircet, dblaikie.
Herald added subscribers: cfe-commits, usaxena95, arphaman, jkorous, MaskRay, 
ilya-biryukov.
Herald added a project: clang.

Fixes an instance of `-Woverloaded-virtual` on GCC.
Clarifies the purpose of this particular function.
Frees up a register that was being used for this pointless parameter of type 
`llvm::NoneType`.

Also eliminate `virtual` from `view(PathRef)` because it is not intended to be 
overridden.

(This is how I propose to address the underlying issue that led to D82617 
, instead of D82617 
. Of course the final call is 
Kadir's-or-Sam's-or-ultimately-certainly-not-mine.)


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D82736

Files:
  clang-tools-extra/clangd/ClangdServer.cpp
  clang-tools-extra/clangd/Preamble.cpp
  clang-tools-extra/clangd/SourceCode.cpp
  clang-tools-extra/clangd/index/Background.cpp
  clang-tools-extra/clangd/support/ThreadsafeFS.cpp
  clang-tools-extra/clangd/support/ThreadsafeFS.h
  clang-tools-extra/clangd/tool/ClangdMain.cpp
  clang-tools-extra/clangd/unittests/ClangdTests.cpp
  clang-tools-extra/clangd/unittests/HeaderSourceSwitchTests.cpp
  clang-tools-extra/clangd/unittests/TestFS.h

Index: clang-tools-extra/clangd/unittests/TestFS.h
===
--- clang-tools-extra/clangd/unittests/TestFS.h
+++ clang-tools-extra/clangd/unittests/TestFS.h
@@ -34,7 +34,7 @@
 class MockFS : public ThreadsafeFS {
 public:
   IntrusiveRefCntPtr
-  view(llvm::NoneType) const override {
+  viewWithDefaultCWD() const override {
 return buildTestFS(Files, Timestamps);
   }
 
Index: clang-tools-extra/clangd/unittests/HeaderSourceSwitchTests.cpp
===
--- clang-tools-extra/clangd/unittests/HeaderSourceSwitchTests.cpp
+++ clang-tools-extra/clangd/unittests/HeaderSourceSwitchTests.cpp
@@ -30,11 +30,11 @@
   FS.Files[FooH];
   FS.Files[Invalid];
   Optional PathResult =
-  getCorrespondingHeaderOrSource(FooCpp, FS.view(llvm::None));
+  getCorrespondingHeaderOrSource(FooCpp, FS.viewWithDefaultCWD());
   EXPECT_TRUE(PathResult.hasValue());
   ASSERT_EQ(PathResult.getValue(), FooH);
 
-  PathResult = getCorrespondingHeaderOrSource(FooH, FS.view(llvm::None));
+  PathResult = getCorrespondingHeaderOrSource(FooH, FS.viewWithDefaultCWD());
   EXPECT_TRUE(PathResult.hasValue());
   ASSERT_EQ(PathResult.getValue(), FooCpp);
 
@@ -45,7 +45,7 @@
 
   FS.Files[FooC];
   FS.Files[FooHH];
-  PathResult = getCorrespondingHeaderOrSource(FooC, FS.view(llvm::None));
+  PathResult = getCorrespondingHeaderOrSource(FooC, FS.viewWithDefaultCWD());
   EXPECT_TRUE(PathResult.hasValue());
   ASSERT_EQ(PathResult.getValue(), FooHH);
 
@@ -54,7 +54,7 @@
   auto Foo2HH = testPath("foo2.HH");
   FS.Files[Foo2C];
   FS.Files[Foo2HH];
-  PathResult = getCorrespondingHeaderOrSource(Foo2C, FS.view(llvm::None));
+  PathResult = getCorrespondingHeaderOrSource(Foo2C, FS.viewWithDefaultCWD());
   EXPECT_TRUE(PathResult.hasValue());
   ASSERT_EQ(PathResult.getValue(), Foo2HH);
 
@@ -64,13 +64,13 @@
 
   FS.Files[Foo3C];
   FS.Files[Foo3HXX];
-  PathResult = getCorrespondingHeaderOrSource(Foo3C, FS.view(llvm::None));
+  PathResult = getCorrespondingHeaderOrSource(Foo3C, FS.viewWithDefaultCWD());
   EXPECT_TRUE(PathResult.hasValue());
   ASSERT_EQ(PathResult.getValue(), Foo3HXX);
 
   // Test if asking for a corresponding file that doesn't exist returns an empty
   // string.
-  PathResult = getCorrespondingHeaderOrSource(Invalid, FS.view(llvm::None));
+  PathResult = getCorrespondingHeaderOrSource(Invalid, FS.viewWithDefaultCWD());
   EXPECT_FALSE(PathResult.hasValue());
 }
 
Index: clang-tools-extra/clangd/unittests/ClangdTests.cpp
===
--- clang-tools-extra/clangd/unittests/ClangdTests.cpp
+++ clang-tools-extra/clangd/unittests/ClangdTests.cpp
@@ -274,7 +274,7 @@
   static Key Secret;
   struct ContextReadingFS : public ThreadsafeFS {
 IntrusiveRefCntPtr
-view(llvm::NoneType) const override {
+viewWithDefaultCWD() const override {
   Got = Context::current().getExisting(Secret);
   return buildTestFS({});
 }
@@ -930,7 +930,7 @@
 : CountStats(CountStats) {}
 
 IntrusiveRefCntPtr
-view(llvm::NoneType) const override {
+viewWithDefaultCWD() const override {
   class StatRecordingVFS : public llvm::vfs::ProxyFileSystem {
   public:
 StatRecordingVFS(IntrusiveRefCntPtr FS,
Index: clang-tools-extra/clangd/tool/ClangdMain.cpp
===
--- clang-tools-extra/clangd/tool/ClangdMain.cpp
+++ clang-tools-extra/clangd/tool/ClangdMain.cpp
@@ -719,7 +719,7 @@
 ClangTidyOptProvider = std::make_unique(
 tidy::ClangTidyGlobalOptions(),
 /* D