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

[James Henderson via Phabricator via llvm-commits]

jhenderson added inline comments.


================
Comment at: llvm/include/llvm/Support/Path.h:466
+///
+/// LLVM \ref is_absolute rules are different.
+///
----------------
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?"


================
Comment at: llvm/include/llvm/Support/Path.h:469-470
+/// @param path Input path.
+/// @param style The style used to define the behavior. Posix style rules are
+/// different from Windows style rules.
+/// @result True if the path is GNU style absolute, false if it is not.
----------------
I think this comment can be more helpful, as suggested in the inline edit. NB, I can't remember doxygen syntax for references to other parameters, so please adjust accordingly.


================
Comment at: llvm/lib/Support/Path.cpp:690
+
+  // Handle '/' which should be true regardless Windows or POSIX.
+  // Handle '\\' on Windows.
----------------



================
Comment at: llvm/unittests/Support/Path.cpp:90
+  // Test tuple <Path, ExpectedPosixValue, ExpectedWindowsValue>.
+  SmallVector<std::tuple<StringRef, bool, bool>, 12> Paths;
+  Paths.emplace_back("", false, false);
----------------
It's probably simpler to just make this an array, and use universal initialization syntax, i.e. something like:
```
std::tuple<StringRef, bool, bool> Paths[] {
  { "", false, false },
  { "/", true, true },
  ...
};
```
N.B: I haven't checked the syntax to ensure I've got this 100% correct.


================
Comment at: llvm/unittests/Support/Path.cpp:95
+  Paths.emplace_back("foo", false, false);
+  Paths.emplace_back("c:", false, true);
+
----------------
jhenderson wrote:
> You also need the following cases, I think:
> ```
> /foo    // paths don't just have to be the directory separator
> \\foo   // ditto (windows only)
> c:\     // ditto (for drive letters)
> !:      // drive char can be a non-letter
> c       // drive char must be followed by ':'
> :       // ':' without drive char is not a drive identifier
> xx:     // drive identifier only accepted for single character
> c:abc\\ // drive identifier without directory separator immediately following
> ```
You missed the `:` only case.


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

https://reviews.llvm.org/D87667