jimingham wrote: I would say this differently. Many clients of the GetNumChildren API are planning to respond the same way to an error and a number of children == 0. Having to embed this "i'm not actually checking the error, I'm just converting it to num-children = 0" snippet looks odd, like "Why are you bothering with errors when you just discard them immediately for "value is 0".
A better title for this use of GetNumChildren would be something like GetNumChildrenErrorsAreZero. That has the virtue of saying what's going on explicitly. Jim > On Mar 7, 2024, at 10:45 AM, Jonas Devlieghere ***@***.***> wrote: > > > @JDevlieghere requested changes on this pull request. > > I'm worried about making it too easy to ignore errors. The whole point of the > llvm::Error class is that you have to check it. There might be a legitimate > reason you don't about the error the call site, but it should very explicit > and easily auditable. If none of the callers care about the error, then it > shouldn't be an Error/Expected in the first place. We have plenty of examples > of that in LLVM. > > I totally understand the need for this as a transitional tool, but I want to > make sure nobody writes new code using this. I can see two ways to make > discourage that: > > Mark the helper as deprecated. This might generate a lot off noise though. > Make it really obvious in the name that this is a temporary or that we're > purposely dropping the error. Either something like FIX_ERROR_HANDLING or > LLDB_DROP_ERROR or something. Logging to the verbose log is better than > nothing, but it doesn't constitute error handling. > PS: The current implementation wraps the LLDB_LOG_* which uses #line and > #file information. Now all those errors will get attributed to this helper > function. > > — > Reply to this email directly, view it on GitHub > <https://github.com/llvm/llvm-project/pull/84219#pullrequestreview-1923251331>, > or unsubscribe > <https://github.com/notifications/unsubscribe-auth/ADUPVW6WTDMOLUAJSNVWUILYXCYVNAVCNFSM6AAAAABEJS3ASKVHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMYTSMRTGI2TCMZTGE>. > You are receiving this because you are on a team that was mentioned. > https://github.com/llvm/llvm-project/pull/84219 _______________________________________________ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
