================
@@ -6754,12 +6754,12 @@ size_t TypeSystemClang::GetIndexOfChildMemberWithName(
llvm::StringRef field_name = field->getName();
if (field_name.empty()) {
CompilerType field_type = GetType(field->getType());
+ std::vector<uint32_t> save_indices = child_indexes;
----------------
Michael137 wrote:
> So there could be several successful iterations, lengthening the vector, but
> final failure. At the point of failure there could be some number of
> no-longer-valid indexes that have been added to the vector, so clearing it is
> the only option (other than saving it before the loop and restoring it when
> the error condition is encountered...).
>
> And (to clarify a bit more), the reason that that is a problem is because at
> line 6759 below, we make a recursive call, but if it fails we don't return an
> error; instead we continue executing in this function (but now with a
> child_indexes vector that would be incorrect if the recursive call hadn't
> cleared it or restored it to its original state).
Right, but if we changed `GetIndexOfChildMemberWithName` to return an
`Expected`, then we'd be able to handle the failure of the recursive call too.
My thinking was that the case where Clang finds a path to a base-class but we
then fail to compute the index to that element feels like a situation where we
should bail out of this function. Although the counterpoint would be, what if
Clang found paths to multiple base-classes, and only one of them failed. Then
yes, we need the backtracking that you implement here. But looking current
implementation, wouldn't we return a bogus result if Clang found multiple
`CXXBasePaths` anyway? It would just add all the indices into `child_indexes`
(it doesn't just pick "the first result" like the FIXME that Pavel points to
suggests).
Correct me if I'm misunderstanding your points though!
I'm happy to go with your simple approach for now and clean up the function as
a follow-up. It looks like there's a few things wrong with it. And the
test-case you added here will be useful to drive that refactor.
I think I prefer @labath's suggestion over mine too. That feels like the most
robust option.
https://github.com/llvm/llvm-project/pull/117808
_______________________________________________
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
