labath added inline comments.
================ Comment at: lldb/test/API/lit.cfg.py:88-91 +def is_configured(attr): + """Return the configuration attribute if it exists and None otherwise. + + This allows us to check if the attribute exists before trying to access it.""" ---------------- JDevlieghere wrote: > JDevlieghere wrote: > > labath wrote: > > > I don't think this name and the implementation are a good match. Given > > > that the usages are a matching the name and not the implementation, maybe > > > just change the implementation to `hasattr(config, attr)`? > > > > > > That said, I'm not sure that `is_configured("foo")` is significantly > > > better than `hasattr(config, "foo")`, so it might be better to just > > > delete the function altogether. > > It does more than just `hasattr(config, "foo")`, is does `hasattr(config, > > "foo") and config.foo`. The option can exist but be set to `False` or > > `None`. > I couldn't think of a good Python way of turning this into a bool instead of > relying not the caller to do so, maybe you do? > It does more than just hasattr(config, "foo"), is does hasattr(config, "foo") > and config.foo. Ah, right. I did miss that distinction. > I couldn't think of a good Python way of turning this into a bool instead of > relying not the caller to do so, maybe you do? Well, there's always the option to say `if hasattr(config, "foo") and config.foo: return True`. However, it's not clear to me why this needs to handle so many cases. I can sort of see how treating a missing attribute and an attribute being set to None in the same way can be handy (for generated config files mainly), and I think it's reasonable for something called `is_configured` to return True there. But I don't think the same treatment should be extended to False or other False-ish thing -- someone had to explicitly set the attribute to this value, so it definitely /is/ configured. For instances where this is needed (treating a False-ish value the same way as a non-existent value), I'd just spell this out in code: ``` if is_configured('llvm_use_sanitizer') and config.llvm_use_sanitizer: ... ``` Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D86821/new/ https://reviews.llvm.org/D86821 _______________________________________________ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits