[Lldb-commits] [PATCH] D76964: Fix an issue where the IgnoreName function was not allowing "Class" to be looked up inside a namespace or other class.

[Raphael Isemann via Phabricator via lldb-commits]

teemperor requested changes to this revision.
teemperor added a comment.
This revision now requires changes to proceed.

LGTM minus some stylistic changes.



================
Comment at: lldb/source/Plugins/ExpressionParser/Clang/ClangASTSource.cpp:582
+    static const ConstString Class_name("Class");
+    if (name == id_name || name == Class_name) {
+      // Only disallow using "id" and "Class" if we are searching from the root
----------------
aprantl wrote:
> For these tiny strings a StringRef `==` comparison is going to be more efficient than constructing and storing a pointer to a ConstString.
There is no need for StringRef as ConstString allows direct comparison against string literals, so this works and is much faster: `name == "id" || name == "Class"`


================
Comment at: lldb/test/API/commands/expression/ignore/TestIgnoreName.py:6
+
+Ticket: https://llvm.org/bugs/show_bug.cgi?id=26790
+"""
----------------
Left over from the original test case (TestCallUserAnonTypedef.py)


================
Comment at: lldb/test/API/commands/expression/ignore/TestIgnoreName.py:32
+                                       add_dependent_modules, error)
+        self.assertTrue(error.Success(), "Make sure our target got created")
+        expr_result = target.EvaluateExpression("a::Class a; a")
----------------
I don't think we usually check the error, but only if target is valid in any other test. So this whole test can just be this (at least after D77197 has landed):
```
lang=python
    def test(self):                                                             
        """Test that we can evaluate an expression that finds something inside  
           a namespace that uses an Objective C keyword.                        
        """                                                                     
        self.build()                                                            
        target = self.dbg.CreateTarget(self.getBuildArtifact("a.out"))          
        self.assertTrue(target, VALID_TARGET)                                   
        self.expect_expr("a::Class x; x", result_type="a::Class")   
```


================
Comment at: lldb/test/API/commands/expression/ignore/main.cpp:7
+int main(int argc, char **argv)
+{
+  a::Class c;
----------------
I think new test files should follow LLVM code style.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D76964/new/

https://reviews.llvm.org/D76964