[Lldb-commits] [PATCH] D57895: Breakpad: auto-detect path style of file entries

[Pavel Labath via Phabricator via lldb-commits]

labath marked 2 inline comments as done.
labath added inline comments.


================
Comment at: include/lldb/Utility/FileSpec.h:250
+  /// unreliable (e.g. "c:\foo.txt" is a valid relative posix path).
+  static llvm::Optional<Style> GuessPathStyle(llvm::StringRef path);
+
----------------
amccarth wrote:
> Given that this implementation is limited to absolute paths ... should it be in such a general purpose class?  
> 
> If so, maybe this restriction be made more obvious by naming the parameter `absolute_path`.  Then people who just look at the signature without reading the comments, or who get a prompt in their IDE are more likely to note the limitation.
Do you have an idea where to place it? Theoretically I could put it into the base SymbolFile class, since all existing users are SymbolFiles, but I am not sure if that makes things any better. Renaming the arg to `absolute_path` sounds good.


================
Comment at: source/Utility/FileSpec.cpp:375
+    return Style::windows;
+  return llvm::None;
+}
----------------
amccarth wrote:
> The code this replaces was returning `Style::native` rather than `Style::None`.  I'm not sure if that will have any implications.
It terms of functionality, it doesn't, since all users then do `getValueOr(Style::native)`. But I am not sure what's the impact on readability -- it seemed better to have a more explicit value for "I don't know" in such a general API, but since all users just revert to native when they can't guess (and I'm not sure if there's anything better they could do), maybe going through `Optional` is overkill. I'd be happy to change it if you think that's better.


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

https://reviews.llvm.org/D57895