[Lldb-commits] [PATCH] D112439: normalize file path when searching the source map

[Adrian Prantl via Phabricator via lldb-commits]

aprantl added inline comments.


================
Comment at: lldb/source/Target/PathMappingList.cpp:33
   // nomalized path pairs to ensure things match up.
   ConstString NormalizePath(ConstString path) {
     // If we use "path" to construct a FileSpec, it will normalize the path for
----------------
Why does this take a ConstString argument if it immediately calls GetStringRef on it?
Could you change this to take a StringRef here or in a follow-up NFC commit?


================
Comment at: lldb/source/Target/PathMappingList.cpp:33-37
   ConstString NormalizePath(ConstString path) {
     // If we use "path" to construct a FileSpec, it will normalize the path for
     // us. We then grab the string and turn it back into a ConstString.
     return ConstString(FileSpec(path.GetStringRef()).GetPath());
   }
----------------
xujuntwt95329 wrote:
> JDevlieghere wrote:
> > Can this function take a `StringRef` and return a `std::string` instead? The amount of roundtrips between `StringRef`s, `ConstString`s and `std::string`s is getting a bit out of hand.
> I agree with you. 
> However, if we change the signature of this function, then we need to do all these conversion from the caller side, seems things are not going better.
> 
> For example
> 
> ```
> void PathMappingList::Append(ConstString path,
>                              ConstString replacement, bool notify) {
>   ++m_mod_id;
>   m_pairs.emplace_back(pair(NormalizePath(path), NormalizePath(replacement)));
>   if (notify && m_callback)
>     m_callback(*this, m_callback_baton);
> }
> ```
> 
> We need to convert `path` to StringRef, or we can change the type of parameter `path`, but this will require more modification from the caller of `Append`
IMO, the best solution would be change this too:
```
void PathMappingList::Append(StringRef path,
                             StringRef replacement, bool notify) {
```


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D112439