I ran check-lldb and I hit one regression in TestFormatters.py, part of what I am seeing is as follows:

@friss we have several bugs, once of which I can reproduce but I have not been able to reduce it to a minimal case yet and the nullptr check is obviously the right to do.

Simplified the checking of symbol being a nullptr

Done with first round.

Updating after https://reviews.llvm.org/D46551 landed

@teemperor @jingham @clayborg I believe now that https://reviews.llvm.org/D46551 is landed the performance concerns should be addressed.

Thank you! LGTM, in general we avoid "large" refactoring changes to avoid polluting the blame list but the changes are relatively local and they are good changes that can catch real bugs in the future. I would like a second set of eyes though.

Modifying copy contructor to be act more intuitively.

Testing both passing as and argument and returning

@friss added second test

@teemperor good call, that is indeed simpler and yes I did not intend that delete.

• Simplifying test
• Fixing unintended deleted test

@aprantl @teemperor I believe I addressed your comments.

This is a good change!

Changed to reflect comments.

• Added comments to test to explain what it is doing.
• Formatting and other minor fixes.

LGTM

@friss makes sense, updated comment.

Updated comment to be more precise.

Fred is correct, I mistakenly thought the parts of the test that were working were being covered elsewhere but that is not the case. So I have reworked this change to instead of skipping the whole test to comment out the inline expressions that are specifically broken.

@friss updated the change to only effect those specifically broken.

Added test

@rsmith I tagged you in this change in case we are missing any implications in using DW_CC_pass_by_reference to do setArgPassingRestrictions(clang::RecordDecl::APK_CannotPassInRegs);

Small updated to test, remove use of printf and associated include.

@friss I had to rework the tests a little but they now cover Objective-C static and non-static methods as well as C and C++.

-Adjusting tests to ensure coverage of Objecive-C static and non-static methods and C and C++

@friss I believe I have addressed your comments

Addressing comments:

• Now applies to all languages not just C++
• When adding locals be more selective on filtering i.e. only filter self and _cmd for Objective C etc...

LGTM

@stella.stamenova I committed a fix, please let me know if this does not address the regression:

In D59847#1446571, @stella.stamenova wrote:

This is causing failures on the windows bot. Please fix it or revert it.

http://lab.llvm.org:8011/builders/lldb-x64-windows-ninja/builds/3066

@friss so far I have not found any other scenarios that relate specifically to this test outside of the one I already have here https://reviews.llvm.org/D59667

In D59847#1443905, @friss wrote:

As we're just adding test coverage, could we add a little more?

• Anonymous enum
• Enum through a typedef
• class enum
• enum declared inside of the function rather than at the top-level
• nested enum in a record type
• enum nested in a templated class
• anything else I haven't thought about...

Fixes based on comments.

LLDB regression test that goes with this fix: https://reviews.llvm.org/D59847

It looks like this breaks the lldb build bots, more specifically it looks like all modules builds are broken, see the following log:

In D59665#1442826, @martong wrote:

In D59665#1442328, @shafik wrote:

@martong your idea does not work b/c default construction DeclarationName() treats it the same as being empty. So if (!Name) is still true.

And did you try moving the push_back to the other scope? I'd expect the the ConflictingDecls to be empty then.

@martong your idea does not work b/c default construction DeclarationName() treats it the same as being empty. So if (!Name) is still true.

In D59665#1439070, @martong wrote:

Hi Shafik,

Thank you for looking into this. This is indeed a bug, because whenever a we encounter an unnamed EnumDecl in the "from" context then we return with a nameconflict error.
However, I think we should fix this differently.
First of all, currently HandleNameConflict just returns the parameter Name it received. This may be unnamed in some cases and thus converts to true. So I think HandleNameConflict should be changed that it would return a default initialized DeclarationName; so once we have anything in the ConflictingDecls then we return with the NameConflict error:

DeclarationName ASTImporter::HandleNameConflict(DeclarationName Name,
                                                DeclContext *DC,
                                                unsigned IDNS,
                                                NamedDecl **Decls,
                                                unsigned NumDecls) {
-  return Name;
+  return DeclarationName();
}

And most importantly, I think we should push into the ConflictingDecls only if the structural match indicates a non-match:

      if (auto *FoundEnum = dyn_cast<EnumDecl>(FoundDecl)) {
        if (isStructuralMatch(D, FoundEnum, true))
          return Importer.MapImported(D, FoundEnum);
+       ConflictingDecls.push_back(FoundDecl);
      }
-
-     ConflictingDecls.push_back(FoundDecl);

I am aware of similar errors like this with other AST nodes. We had a patch in our fork to fix that in January (https://github.com/Ericsson/clang/pull/569/files) I am going to prepare a patch from that cause I see now this affects you guys in LLDB too.