This is an archive of the discontinued LLVM Phabricator instance.

Refactor ClangASTContext::AddEnumerationValueToEnumerationType() to remove redundant parameter which can be calculated from other parameter
ClosedPublic

Authored by shafik on Nov 1 2018, 2:53 PM.

Details

Summary

Refactor ClangASTContext::AddEnumerationValueToEnumerationType() to remove redundant parameter which can be calculated from other parameter.

Both calling sites of the sites were incorrectly calculating the qualtype as the underlying type unconditionally irrespective of whether the enum was unscoped or scoped

Diff Detail

Event Timeline

shafik created this revision.Nov 1 2018, 2:53 PM
davide added inline comments.Nov 1 2018, 3:14 PM
packages/Python/lldbsuite/test/expression_command/radar_43822994/TestScopedEnumType.py
42

This clearly looks like can be an inline test (and probably would make the thing more readable, IMHO).

This looks nice, just added some minor comments below. Otherwise LGTM after Davide's point is addressed.

include/lldb/Symbol/ClangASTContext.h
909

Can we pass enum_type as const ref?

source/Symbol/ClangASTContext.cpp
8851

(Same as above) Can we have enum_type as a const ref?

zturner added a subscriber: zturner.Nov 2 2018, 2:50 PM

This function is called in SymbolFile/NativePDB as well.

LGTM. lldbassert to check that AST in "enum_type" is the same as "this" since we now can after switching to CompilerType as arg instead of opaque clang type pointer.

include/lldb/Symbol/ClangASTContext.h
909

CompilerType instances are two pointers. Pretty cheap to copy.

source/Symbol/ClangASTContext.cpp
8851

CompilerType instances are two pointers. Pretty cheap to copy.

8853

lldbassert that the AST in "enum_type" is the same as "this"?

zturner added a subscriber: shafik.Nov 7 2018, 2:28 PM

I meant the folder. It’s the first result in your search results. Just
curious, why does your build successfully complete without fixing that
instance?

shafik added a comment.Nov 7 2018, 2:31 PM

@zturner I was out of date when I posted this diff but I have that file updated locally and it will show up when I update the diff.

jingham requested changes to this revision.Nov 7 2018, 3:17 PM

Looking at the extant API's we do seem to prefer "const CompilerType &" over "const CompilerType" so it seems more consistent to choose that. Other than that, this looks fine to me.

include/lldb/Symbol/ClangASTContext.h
909

We're not entirely consistent, but a quick glance through headers show us almost always choosing to pass "const CompilerType &" over "const CompilerType".

packages/Python/lldbsuite/test/expression_command/radar_43822994/TestScopedEnumType.py
42

The form of test to use is up to the test writer, seems to me. This test is not at all hard to read.

This revision now requires changes to proceed.Nov 7 2018, 3:17 PM
shafik updated this revision to Diff 173083.Nov 7 2018, 4:32 PM
shafik marked 8 inline comments as done.

Made changes based on comments.

shafik added a comment.Nov 7 2018, 4:33 PM

@jingham @zturner @teemperor @clayborg I believe I have addressed all the comments.

include/lldb/Symbol/ClangASTContext.h
909

That was a good catch, thank you!

zturner added inline comments.Nov 7 2018, 4:36 PM
include/lldb/Symbol/ClangASTContext.h
909

Definitely passing by const value doesn't make sense, but I also think passing by value is what we should do going forward. It doesn't necessarily have to be in this patch, but I don't see any reason to pass by reference in general, and it just adds an extra level of pointer indirection for no real benefit IMO.

jingham accepted this revision.Nov 7 2018, 5:06 PM

Looks good to me.

This revision is now accepted and ready to land.Nov 7 2018, 5:06 PM
zturner accepted this revision.Nov 7 2018, 5:07 PM
This revision was automatically updated to reflect the committed changes.

@zturner I would not be against discussing using pass by value for small objects going forward. I don't currently have a good feeling for at what sizes/data types the right trade-off is at though.