Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

[AST] Enhance Struct Decl #117

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

[AST] Enhance Struct Decl #117

wants to merge 2 commits into from

Conversation

LFsWang
Copy link
Contributor

@LFsWang LFsWang commented Sep 1, 2021

No description provided.

@codecov
Copy link

codecov bot commented Sep 1, 2021 •
edited

Codecov Report

Merging #117 (d748632) into master (821b1f6) will decrease coverage by 1.74%.
The diff coverage is 61.46%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #117      +/-   ##
==========================================
- Coverage   66.47%   64.73%   -1.75%     
==========================================
  Files          98      106       +8     
  Lines       13679    14095     +416     
  Branches     2041     2162     +121     
==========================================
+ Hits         9093     9124      +31     
- Misses       3567     3888     +321     
- Partials     1019     1083      +64
Impacted Files Coverage Δ
include/soll/AST/Stmt.h 98.03% <0.00%> (-1.97%) ⬇️
include/soll/Sema/Sema.h 90.00% <ø> (ø)
include/soll/Sema/GlobalContent.h 21.42% <21.42%> (ø)
lib/AST/DeclVisitor.cpp 80.85% <33.33%> (-3.60%) ⬇️
lib/AST/Expr.cpp 60.21% <33.33%> (-5.38%) ⬇️
include/soll/AST/Decl.h 80.58% <47.82%> (-9.55%) ⬇️
include/soll/Sema/DeclarationContainer.h 50.00% <50.00%> (ø)
lib/AST/Decl.cpp 67.96% <51.72%> (-3.94%) ⬇️
include/soll/Sema/NameAndTypeResolver.h 55.55% <55.55%> (ø)
include/soll/Sema/ReferenceResolver.h 57.14% <57.14%> (ø)
... and 24 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 821b1f6...d748632. Read the comment docs.

@LFsWang LFsWang changed the title [WIP] [AST] Enhance Struct Decl [AST] Enhance Struct Decl Sep 3, 2021
@hydai
Copy link
Member

hydai commented Sep 7, 2021

Please rebase to the latest master

@LFsWang LFsWang changed the title [AST] Enhance Struct Decl [WIP] [AST] Enhance Struct Decl Sep 9, 2021
@lgtm-com
Copy link

lgtm-com bot commented Sep 9, 2021

This pull request introduces 3 alerts when merging c3c0042 into 821b1f6 - view on LGTM.com

new alerts:

  • 2 for Declaration hides parameter
  • 1 for Missing return statement

@lgtm-com
Copy link

lgtm-com bot commented Sep 15, 2021

This pull request introduces 2 alerts when merging 6313291 into 821b1f6 - view on LGTM.com

new alerts:

  • 2 for Declaration hides parameter

@LFsWang LFsWang force-pushed the NameAndTypeResolver branch 4 times, most recently from 7bd569b to d1fc314 Compare September 15, 2021 16:32
@hydai hydai self-requested a review September 16, 2021 05:37
@LFsWang LFsWang force-pushed the NameAndTypeResolver branch 3 times, most recently from f9fd595 to 0d0d5a2 Compare October 5, 2021 18:56
@LFsWang LFsWang force-pushed the NameAndTypeResolver branch 2 times, most recently from 1b9e54d to 9dfd770 Compare October 13, 2021 17:39
@LFsWang LFsWang changed the title [WIP] [AST] Enhance Struct Decl [AST] Enhance Struct Decl Oct 20, 2021
hydai
hydai requested changes Nov 25, 2021
View reviewed changes
include/soll/ADT/STLExtras.h
template <class T, class U>
std::vector<T> &operator+=(std::vector<T> &A, U &B) {
for (auto const &I : B)
A.push_back(T(I));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should have {} in a loop statement.

include/soll/Sema/DeclarationContainer.h Outdated
std::vector<const DeclarationContainer *> InnerContainers;
std::map<llvm::StringRef, std::vector<const Decl *>> Declarations;
std::map<llvm::StringRef, std::vector<const Decl *>> InvisibleDeclarations;
// std::vector<std::tuple<std::string, langutil::SourceLocation const*>>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do we need to keep the comments part?

lib/Sema/ReferenceResolver.cpp Outdated
void ReferencesResolver::visit(Identifier &I) {
auto Decls = Resolver.nameFromCurrentScope(I.getName());
if (Decls.empty()) {
// Bypass too next pass
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't understand this comment.

lib/Sema/SemaNameAndTypeResolver.cpp Outdated

// We use "invisible" for both inactive variables in blocks and for members
// invisible in contracts. They cannot both be true at the same time.
// assert(!(Inactive && D->getVisibility() != Decl::Visibility::External));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like we need this assert. However, this is comment out. Why?

lib/Sema/SemaNameAndTypeResolver.cpp Outdated

if (!DeclCont->registerDeclaration(D, Name, &D->getLocation(), Inactive,
false)) {
// Bypass too next pass
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ditto.

@LFsWang LFsWang force-pushed the NameAndTypeResolver branch 3 times, most recently from 3e9de36 to da831d9 Compare December 8, 2021 15:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@hydai hydai hydai requested changes

Requested changes must be addressed to merge this pull request.

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@LFsWang @hydai