Page MenuHomePhabricator

Fix an issue where the IgnoreName function was not allowing "Class" to be looked up inside a namespace or other class.
Needs ReviewPublic

Authored by clayborg on Mar 27 2020, 6:12 PM.

Details

Summary

Objective C may or may not be enabled when running expressions. There was code that was no allowing "Class" or "id" to be looked up in expressions, but it was preventing these names from being able to be looked up in a namespace or class context. This fixes the issue by passing the namespace_decl along and testing it to make sure we only stop ones at the root namespace level.

Diff Detail

Event Timeline

clayborg created this revision.Mar 27 2020, 6:12 PM
Herald added a project: Restricted Project. · View Herald TranscriptMar 27 2020, 6:12 PM
aprantl added inline comments.Mar 30 2020, 9:44 AM
lldb/source/Plugins/ExpressionParser/Clang/ClangASTSource.cpp
582

For these tiny strings a StringRef == comparison is going to be more efficient than constructing and storing a pointer to a ConstString.

lldb/test/API/commands/expression/ignore/TestIgnoreName.py
37

assertEqual(expr_result.GetValue(), None, ...) is better here, because it will print the result of expr_result.GetValue() in the failure case.

39

same here

shafik added a subscriber: shafik.Mar 30 2020, 10:28 AM

You also mentioned the id case failing as well. We should add that case to the test as well.

teemperor requested changes to this revision.Apr 1 2020, 12:28 AM

LGTM minus some stylistic changes.

lldb/source/Plugins/ExpressionParser/Clang/ClangASTSource.cpp
582

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"

lldb/test/API/commands/expression/ignore/TestIgnoreName.py
7

Left over from the original test case (TestCallUserAnonTypedef.py)

33

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):

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")
lldb/test/API/commands/expression/ignore/main.cpp
8

I think new test files should follow LLVM code style.

This revision now requires changes to proceed.Apr 1 2020, 12:28 AM
clayborg updated this revision to Diff 254352.Apr 1 2020, 4:52 PM

Fixed patch reviewer suggestions and added a test for "id".