jingham accepted this revision. jingham added a comment. GetPropertyAtIndexAsArg isn't actually documented, so we don't know what the original intent for the return value was. But in almost all the cases where we look up a property using this function, when we use the enum directly we discard the result. In the only other case (GetUserSpecifiedTrapHandlerNames) were we do return the result of the GetProperty... function, the return result of the GetUser... function is always discarded. So it seems pretty clear that the result was just supposed to be "couldn't find the property". It's also weird to conflate "couldn't find property" with "property has an empty value".
But then that leads one to wonder why TargetProperties::GetRunArguments is returning a bool not a void. Does that relay any useful information? It's not like we're going to remove the property at ePropertyRunArgs but leave the GetRunArguments function around, thus rendering useful the check on return value? And if the intent was to return info about elements in the array, returning the number of elements makes some sense, but an "empty" bool is odd and not in line with the way we usually do things. The other question is - given that this function used to mean "true if there was more than one arg, false if there were none" - whether there was any reason why when someone reset the target run-args to an empty args set we wouldn't want to update the launch info. That really seemed the intent of the original code. Was there some instance where we would inadvertently empty the run args property w/o wanting to update the next run of that target? Anyway, I couldn't think of any good reason for that, and it clearly has this undesirable side effect. So I think that was just a case of "it has a return value I should check it" rather than intending to do something special for empty run args. TL;DR This looks okay to me. I think it would be good to also change GetRunArguments to remove the now useless bool return value, but the patch is also fine without that. I think you should keep the other changes, it would be confusing for this to be inconsistent and it's not like those changes are going to have subtle bugs in the change. I'd keep the other two changes. We should be consistent, and it's not like this change itself is likely to have subtle bugs. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D126057/new/ https://reviews.llvm.org/D126057 _______________________________________________ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
