Page MenuHomePhabricator

Fix handling of CreateTemplateParameterList when there is an empty pack
ClosedPublic

Authored by shafik on Mon, Jan 28, 4:25 PM.

Details

Summary

When we are creating a ClassTemplateSpecializationDecl in ParseTypeFromDWARF(...) we are not handling the case where variadic pack is empty in the specialization. This patch handles that case and adds a test to prevent future regressions.

Diff Detail

Repository
rL LLVM

Event Timeline

shafik created this revision.Mon, Jan 28, 4:25 PM
aprantl added inline comments.Mon, Jan 28, 4:36 PM
packages/Python/lldbsuite/test/expression_command/radar_47565290/Makefile
1 ↗(On Diff #183982)

Could you give the directory a more descriptive name instead of radar_47565290? It's fine to mention a rdar:// link in the commit message, but for most people radar numbers are completely opaque.

packages/Python/lldbsuite/test/expression_command/radar_47565290/TestClassTemplateSpecializationParametersHandling.py
2 ↗(On Diff #183982)

Same here, mentioning the radar in the commit message should be sufficient. This forces us to write better/more complete comments, too :-)

source/Symbol/ClangASTContext.cpp
1562 ↗(On Diff #183982)

does this get more or less readable if we replace SourceLocation() with {}?

1572 ↗(On Diff #183982)

Looks like this is the same code as above. Could this be organized differently to avoid duplicating the code? (I didn't look whether these are all different constructors, so maybe this is already as good as it gets)

aprantl added inline comments.Mon, Jan 28, 4:38 PM
packages/Python/lldbsuite/test/expression_command/radar_47565290/TestClassTemplateSpecializationParametersHandling.py
9 ↗(On Diff #183982)

I don't think these are used

19 ↗(On Diff #183982)

test_class_template_specialization ?

20 ↗(On Diff #183982)

This needs to be updated, too.

aprantl added inline comments.Mon, Jan 28, 4:41 PM
packages/Python/lldbsuite/test/expression_command/radar_47565290/TestClassTemplateSpecializationParametersHandling.py
23 ↗(On Diff #183982)

FYI, Python allows you to write this as self.target, self.process, _, bkpt = without the parentheses, too.
Since you aren't any of the return values, you could just call the function and ignore the return value.

davide added a subscriber: davide.Mon, Jan 28, 6:56 PM
davide added inline comments.
packages/Python/lldbsuite/test/expression_command/radar_47565290/TestClassTemplateSpecializationParametersHandling.py
5–6 ↗(On Diff #183982)

I think this is not needed.

source/Symbol/ClangASTContext.cpp
1572 ↗(On Diff #183982)

Why this code block became conditional of if (identifier_info) ?
I understand the size check above, but can you elaborate on the need for this?

teemperor added inline comments.Tue, Jan 29, 9:58 AM
packages/Python/lldbsuite/test/expression_command/radar_47565290/main.cpp
1 ↗(On Diff #183982)

Maybe I miss something, but this could be simpler I think? E.g. like this:

template <typename N, class... P>
struct A {
    int foo() { return 1;}
};

int main() {
  A<int> b;
  return b.foo(); // break here
}
source/Symbol/ClangASTContext.cpp
1558 ↗(On Diff #183982)

I think !template_param_infos.packed_args->args.empty() is more LLVM-ish.

1562 ↗(On Diff #183982)

We have SourceLocation() everywhere in clang, so it is at least more consistent this way IMHO.

shafik updated this revision to Diff 184173.Tue, Jan 29, 2:21 PM
shafik marked 19 inline comments as done.

Addressed comments, including

  • Renamed test
  • Reduced test size
  • Stream-lined code in CreateTemplateParameterList(...)

@aprantl @teemperor @davide thank you for the review, some great catches. I believe I have addressed the comments.

packages/Python/lldbsuite/test/expression_command/radar_47565290/Makefile
1 ↗(On Diff #183982)

Makes sense, there are a bunch of tests like this. I did not check how old they were though.

packages/Python/lldbsuite/test/expression_command/radar_47565290/TestClassTemplateSpecializationParametersHandling.py
20 ↗(On Diff #183982)

I will just remove it since the top comment should be sufficient.

packages/Python/lldbsuite/test/expression_command/radar_47565290/main.cpp
1 ↗(On Diff #183982)

Good catch, I was modeling the code that was the original source of the problem. I had simplified it a lot but yes I could have kept going it appears.

source/Symbol/ClangASTContext.cpp
1562 ↗(On Diff #183982)

I also think using {} here would obscure more then help, using {} in return statements they make much more sense usually though.

1572 ↗(On Diff #183982)

I don't think I need it, I thought I did but looking at it over again I don't think that was justified.

1572 ↗(On Diff #183982)

It was not obvious at first but I was able to simplify a bit.

aprantl accepted this revision.Tue, Jan 29, 2:29 PM
This revision is now accepted and ready to land.Tue, Jan 29, 2:29 PM
teemperor accepted this revision.Tue, Jan 29, 2:34 PM

LGTM, thanks for the patch!

This revision was automatically updated to reflect the committed changes.