[PATCH] D159471: [DWARFVerifier] Allow ObjectiveC names in dwarf_debug tables

[David Blaikie via Phabricator via llvm-commits]

dblaikie added inline comments.


================
Comment at: llvm/lib/DebugInfo/DWARF/DWARFVerifier.cpp:1367
+        // a vector resize which may destroy the StringRef memory.
+        Result.push_back(StrippedName->str());
+    }
----------------
fdeazeve wrote:
> dblaikie wrote:
> > Technically, that'd be the behavior of this code even if it used `*` instead of `->str()`, right? Since `push_back` takes a `T&&` parameter the `StringRef`->`std::string` implicit conversion would happen first, before the function call.
> > 
> > So I'm not sure the use of `->str()` is sufficiently more explicit to make it clear what's going on here. Maybe the comment is sufficient and we could use `*`, or maybe it isn't, and a named temporary (`std::string X = *y; func(std::move(X);`) might be better, or an explicit cast (I guess that's no better than the `->str()`)?
> > 
> > I'm not feeling strongly about any of this - if the comment and the slightly different syntax feels like the right tradeoff to you folks, I won't stand in the way.
> Oh, you are right; I could have done just `*` instead of `->str()`, I totally missed that! I can probably simplify the comment a bit with that in mind.
> 
> I think the only important thing we emphasize with the comment is that we need to push instead of emplacing the StringRef, as this is a trap a well-intentioned developer could fall into during future code changes.
> 
> Personally I don't think the temporary would improve things a lot, as in the future someone could think "oh, let me remove this temporary that is just used once", not realizing it was done because of this subtle problem.
Fair enough - probably just leave/rephrase the comment (to try to reduce @aprantl's confusion) and replace the `->str()` with `*`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D159471