labath added a comment. In D131539#3712675 <https://reviews.llvm.org/D131539#3712675>, @JDevlieghere wrote:
> In D131539#3711835 <https://reviews.llvm.org/D131539#3711835>, @labath wrote: > >> In D131539#3711518 <https://reviews.llvm.org/D131539#3711518>, @JDevlieghere >> wrote: >> >>> In D131539#3711488 <https://reviews.llvm.org/D131539#3711488>, @kastiglione >>> wrote: >>> >>>> this diff has made me wonder: should we have a `NoDebugInfoTestCase` that >>>> can be used by any test, and would replace assigning to >>>> `NO_DEBUG_INFO_TESTCASE`? >>> >>> I was wondering the same thing. I decided against it because we already >>> have `NO_DEBUG_INFO_TESTCASE` (test level) and `@no_debug_info_test` >>> (function level) and I didn't want to add yet another option. Additionally, >>> there's a few other patters that are common for the Python API tests (e.g. >>> the `self.source` and `self.line`) that could be moved up into the base >>> class in a follow up patch. >> >> Note that we already kind of have that. The `Base` test class is the base of >> all our tests and does not support automatic test replication. Tests which >> inherit directly from that (lldb-server and lldb-vscode tests) for instance, >> don't get the replication even though they don't use >> `NO_DEBUG_INFO_TESTCASE`. The `TestBase` class (which we use for the >> "normal" SB API tests) adds a bunch of SB utility functions *and* it adds >> the ability to replicate tests. I think that separating the two parts would >> be completely reasonable, and would remove the need for most/all of the uses >> of `NO_DEBUG_INFO_TESTCASE` and `@no_debug_info_test`. > > The ability to replicate tests is not even part of `TestBase` I think. Except > for the attribute, it's all handled by the `LLDBTestCaseFactory`. Well.. yes, the actual code lives there, but if you think about it from the POV of a test case author, then he has the choice of either inheriting from Base (and getting no replication), or from TestBase (and getting the replication unless he disables it). I would say that the LLDBTestCaseFactory is an implementation detail that should not be visible to the outside world. > If you think it's better to go the test case route (in favor of > NO_DEBUG_INFO_TESTCASE) we can pull up everything but the factory and > property from `TestBase` into `Base` (as you outlined) or keep them separate > and just move the factory and property down to a (third) child class. I wouldn't want to put everything into Base, precisely because we have these gdb-server, vscode, etc., which don't operate on the SB API, and that functionality makes no sense there. > Either way we should rename those 2 (3) classes to something more meaningful. Oh yes. > @labath: was your comment meant as a suggestion (i.e. we should make this > change) or just informative in reply to Dave's question (i.e. we could make > this change if we needed it)? Something in between, I guess. I would like it to work that way, but I'm not going to make anyone do that right now. It's also a part of a larger discussion of whether the debug info replication should be opt-in or opt-out. A separate class makes more sense for an opt-in world, whereas the status quo is kind of geared towards an opt-out mechanism. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D131539/new/ https://reviews.llvm.org/D131539 _______________________________________________ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
