[PATCH] D159471: [DWARFVerifier] Allow ObjectiveC names in dwarf_debug tables
Felipe de Azevedo Piovezan via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Mon Sep 18 03:55:05 PDT 2023
fdeazeve 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());
+ }
----------------
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.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D159471/new/
https://reviews.llvm.org/D159471
More information about the llvm-commits
mailing list