[PATCH] D155723: [DWARFVerifier] Allow simplified template names in debug_name

[David Blaikie via Phabricator via llvm-commits]

dblaikie added inline comments.


================
Comment at: llvm/lib/DebugInfo/DWARF/DWARFVerifier.cpp:1363
+              StripTemplateParameters(Result.back()))
+        Result.push_back(*StrippedName);
+    }
----------------
avl wrote:
> fdeazeve wrote:
> > dblaikie wrote:
> > > fdeazeve wrote:
> > > > avl wrote:
> > > > > fdeazeve wrote:
> > > > > > avl wrote:
> > > > > > > nit: other places use emplace_back.
> > > > > > Other places have a `const char*`, which is why they emplace, not push. In the grand scheme of things, either will generate the same code, but I think the `push` here is the more expressive call: we are communicating that we already have a StringRef. Do you agree?
> > > > > I am not sure that I follow... Other places might also use push_back even if they have const char* and vice versa this place can use emplace_back. It is just looks more uniformly if the single variant is used. 
> > > > > Other places might also use push_back even if they have const char*
> > > > 
> > > > Right, that's true because the `StringRef(const char*)` constructor is not explicit, but it is not generally true for all types. If we `push_back(const char*)`, the StringRef is being implicitly constructed on the **call** to push_back; when we `emplace_back(const char*)`, the StringRef is constructed inside the implementation of `emplace_back`.
> > > > 
> > > > But as I mentioned previously, this discussion is largely irrelevant for a simple type like StringRef, so I don't feel strongly either way. I'll use `emplace_back`!
> > > FWIW - +1 from me, that we should prefer push_back/non-direct init (`X y = z;` rather than `X y(z);`) whenever possible, because those forms are "less powerful" and make the code easier to read because as a reader you don't have to worry about more powerful operations happening when you see that syntax/function name. (eg: if I see `push_back(ptr)` I don't have to worry that that's taking ownership - if the contianer was over unique_ptrs then you'd have to pass `emplace_back(ptr)` and it'd be a clearer marker that memory ownership was happening)
> > I think given the trade-offs discussed here of consistency with other methods vs expressiveness of the intent behind {push/emplace}_back, I am going to revert to what I had previously and use push_back.
> > 
> > @dblaikie do you have other concerns with this patch?
> This is a minor issue and I do not ask for doing this way.
> Though, I want to clarify my point. In this case, using 
> emplace_back(const char*) does not indicate that ownership
> is taken. emplace_back(const char*) and emplace_back(StringRef) 
> are equal in that sense. When we have emplace_back() and push_back()
> intermixed it looks like there is an important difference
> in their usages. My original request was to do things in the same
> way. All emplace_back(as in original code) or all push_back.
> 
> I think the intention for using emplace_back here was caused by
> the desire to not call two constructors: one in push_back parameters (StringRef(const char*))
> and another copy constructor inside push_back when placing an item into the vector.
> Having all methods to be emplace_back shows that intention.
> 
> Having all methods to be push_back would show the intention that David has mentioned: using "less powerful" forms.
if other code is using `emplace_back` where `push_back` is valid, then I think we should switch that code to use `push_back` - patches welcome?

(an extra StringRef ctor/move doesn't seem enough to justify a difference - that's pretty trivial)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D155723