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.

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

Constraint checks C727 to C730 and most constraints related to attributes #1084

Merged
merged 1 commit into from Mar 24, 2020

Conversation

psteinfeld
Copy link
Collaborator

The full list of checked constraints is C727, C728, C729, C730, C743, C755, C759, C778,
and C1543.

I added a function to tools.cpp to check to see if a symbol name is the name
of an intrinsic type.

The biggest change was to resolve-names.cpp to check to see if attributes were
either duplicated or in conflict with each other. I changed all locations
where attributes were set to check for duplicates or conflicts.

I also added tests for all checks and annotated the tests and code with the
numbers of the constraints being tested/checked.

Comment on lines 678 to 680
static constexpr std::size_t intrinsicNameCount{6};
static const std::string intrinsicNames[intrinsicNameCount]{
"integer", "real", "doubleprecision", "complex", "character", "logical"};
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
static constexpr std::size_t intrinsicNameCount{6};
static const std::string intrinsicNames[intrinsicNameCount]{
"integer", "real", "doubleprecision", "complex", "character", "logical"};
static const std::string intrinsicNames[]{
"integer", "real", "doubleprecision", "complex", "character", "logical"};
std::size_t intrinsicNameCount{
sizeof(intrinsicNames) / sizeof(intrinsicNames[0])};

This avoids the magic number 6.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Using std::array would be even nicer. Or null termination, which I tend to use myself.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Thanks, Tim. I couldn't figure out how to do this.

lib/Semantics/tools.cpp Show resolved Hide resolved
lib/Semantics/resolve-names.cpp Outdated Show resolved Hide resolved
lib/Semantics/resolve-names.cpp Outdated Show resolved Hide resolved
lib/Semantics/resolve-names.cpp Show resolved Hide resolved
!ERROR: No intrinsic or user-defined ASSIGNMENT(=) matches operand types LOGICAL(4) and REAL(4)
f = 1.0
end
end
Copy link
Collaborator

Choose a reason for hiding this comment

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

What happened to the test previously in resolve76.f90?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Thanks for catching this. I mistakenly overwrote it when I merged from master into my branch. I'll restore it.

!ERROR: Attribute 'TARGET' cannot be used more than once
real, target, target :: targetVar
!ERROR: Attribute 'VOLATILE' cannot be used more than once
real, volatile, volatile :: volatileVar
Copy link
Collaborator

Choose a reason for hiding this comment

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

There should be some tests where the duplicate attributes are not in the same statement.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Wow, I didn't even realize that this was possible. I'll add some tests.

bool AttrsVisitor::IsDuplicateAttr(Attr attrName) {
if (attrs_->test(attrName)) {
Say(currStmtSource().value(),
"Attribute '%s' cannot be used more than once"_err_en_US,
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why a fatal error? It's not going to hurt anything.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Thanks for pointing this out. I agree with you. I just checked, and duplicate attributes don't seem to be noticed by pgf90, but they're fatal errors for both IBM and Intel.

I'll change the duplicates to warnings.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Actually, some attributes such as CODIMENSION, EXTENDS, DIMENSION convey extra information. It seems appropriate to me that duplicating these attributes should be an error.

Copy link
Collaborator

Choose a reason for hiding this comment

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

EXTENDS is an optional part of the definition of a derived type, not an attribute of an entity.

I'm not saying that we should allow all possible duplicated attributes (besides the ones that the standard says we have to allow).

…ted to attributes

The full list of constraints is C727, C728, C729, C730, C743, C755, C759, C778,
and C1543.

I added a function to tools.cpp to check to see if a symbol name is the name
of an intrinsic type.

The biggest change was to resolve-names.cpp to check to see if attributes were
either duplicated or in conflict with each other.  I changed all locations
where attributes were set to check for duplicates or conflicts.

I also added tests for all checks and annotated the tests and code with the
numbers of the constraints being tested/checked.
@psteinfeld psteinfeld merged commit 6dbfb80 into master Mar 24, 2020
@psteinfeld psteinfeld deleted the ps-typechecks branch March 24, 2020 17:13
swift-ci pushed a commit to apple/llvm-project that referenced this pull request Apr 9, 2020
…nts related to attributes

The full list of constraints is C727, C728, C729, C730, C743, C755, C759, C778,
and C1543.

I added a function to tools.cpp to check to see if a symbol name is the name
of an intrinsic type.

The biggest change was to resolve-names.cpp to check to see if attributes were
either duplicated or in conflict with each other.  I changed all locations
where attributes were set to check for duplicates or conflicts.

I also added tests for all checks and annotated the tests and code with the
numbers of the constraints being tested/checked.

Original-commit: flang-compiler/f18@3f30e8a
Reviewed-on: flang-compiler/f18#1084
swift-ci pushed a commit to apple/llvm-project that referenced this pull request Apr 9, 2020
…r/ps-typechecks

Constraint checks C727 to C730 and most constraints related to attributes

Original-commit: flang-compiler/f18@6dbfb80
Reviewed-on: flang-compiler/f18#1084
mem-frob pushed a commit to draperlaboratory/hope-llvm-project that referenced this pull request Oct 7, 2022
…nts related to attributes

The full list of constraints is C727, C728, C729, C730, C743, C755, C759, C778,
and C1543.

I added a function to tools.cpp to check to see if a symbol name is the name
of an intrinsic type.

The biggest change was to resolve-names.cpp to check to see if attributes were
either duplicated or in conflict with each other.  I changed all locations
where attributes were set to check for duplicates or conflicts.

I also added tests for all checks and annotated the tests and code with the
numbers of the constraints being tested/checked.

Original-commit: flang-compiler/f18@3f30e8a
Reviewed-on: flang-compiler/f18#1084
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants