[PATCH] D87667: [Support/Path] Add path::is_gnu_absolute
Vinicius Tinti via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Mon Sep 21 13:31:59 PDT 2020
tinti marked an inline comment as done.
tinti added a comment.
I have done several rewords.
I think it is better now. I hope I haven't missed more suggestions.
================
Comment at: llvm/include/llvm/Support/Path.h:466
+///
+/// LLVM \ref is_absolute rules are different.
+///
----------------
jhenderson wrote:
> 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.
I missed the move part.
================
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},
----------------
jhenderson wrote:
> 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).
My bad. I missed this one too.
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D87667/new/
https://reviews.llvm.org/D87667
More information about the llvm-commits
mailing list