[PATCH] D125778: [llvm-dva] 03 - Logical elements

[Pavel Samolysov via Phabricator via llvm-commits]

psamolysov added inline comments.


================
Comment at: llvm/include/llvm/DebugInfo/LogicalView/Core/LVReader.h:109
+  StringRef getFilename() const { return InputFilename; }
+  void setFilename(std::string Name) { InputFilename = Name; }
+  StringRef getFileFormatName() const { return FileFormatName; }
----------------
probinson wrote:
> psamolysov wrote:
> > 
> Should Name be a reference parameter? Passing std::string by value could result in a creating/destroying a temp copy.
Using `std::move` after getting a parameter by value has the following advantage: if the actual parameter (`Name` in our case) is a temporary or also moved value (`rvalue`?) so:

```
LVReader r;
r.setFilename(getFilename(...));
```

or 

```
LVReader r;
std::string name = getFilename(...);
r.setFilename(std::move(name));
```

the move constructor of `std::string` will be called twice and the copy constructor will not be called at all. If we use pass-by-const-reference for the `Name` parameter in the `setFilename` method, the copy constructor will be called but only once. In the general case, move constructor is much cheaper and two calls of a move constructor could be more preferable than even a single call of a copy constructor but for `std::string` this is not obvious due to short string optimization. If filenames are usually short strings, shorter than 32 characters, using pass by reference for the `Name` looks better for me.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D125778