[PATCH] D87667: [Support/Path] Add path::is_gnu_absolute

[James Henderson via Phabricator via llvm-commits]

jhenderson added a comment.

In D87667#2285036 <https://reviews.llvm.org/D87667#2285036>, @tinti wrote:

> - Drop the "_style" in the name of the function.
> - Add ":" test.
> - Add comment about LLVM is_absolute.
> - Add Vector constructor.
>
> Now I think it is better to call it "is_gnu_absolute". The "style" is already used for style::{posix,windows,native}.
> After reading the comments it was confusing. It might sound like we were introducing a new style::gnu which is not the case.
>
> I can revert back if you don't agree. Please let me know.

I'd go with `is_absolute_gnu` instead. I think it reads a little better, personally, if you are dropping "style" (I don't mind either way on whether "style" is present in the name).



================
Comment at: llvm/include/llvm/Support/Path.h:466
+///
+/// LLVM \ref is_absolute rules are different.
+///
----------------
jhenderson wrote:
> It might be worth adding to the `is_absolute` comment expanding the LLVM rules, so that people don't read this and go "okay, what are the differences?"
As mentioned in my previous comment, I think you should move the comment to the `is_absolute` function. I'd also remove reference to `LLVM`, since this applies to the standard C++17 `is_absolute` function too, I believe.


================
Comment at: llvm/include/llvm/Support/Path.h:458
 
+/// Is path GNU absolute?
+///
----------------



================
Comment at: llvm/include/llvm/Support/Path.h:481
+/// means to derive the style from the host.
+/// @result True if the path is GNU absolute, false if it is not.
+bool is_gnu_absolute(const Twine &path, Style style = Style::native);
----------------



================
Comment at: llvm/unittests/Support/Path.cpp:90
+  // Test tuple <Path, ExpectedPosixValue, ExpectedWindowsValue>.
+  const SmallVector<std::tuple<StringRef, bool, bool>, 13> Paths = {
+      {"", false, false},  {"/", true, true},      {"/foo", true, true},
----------------
Why are you still using `SmallVector` here? I suggested an array before, and I don't see any benefit to the `SmallVector` (we don't need vector operations for this test).


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

https://reviews.llvm.org/D87667