Skip to content

Remove parsing support for old bounds cast expression syntax.#468

Merged
dtarditi merged 3 commits into
masterfrom
issue448
Mar 22, 2018
Merged

Remove parsing support for old bounds cast expression syntax.#468
dtarditi merged 3 commits into
masterfrom
issue448

Conversation

@dtarditi

@dtarditi dtarditi commented Mar 21, 2018

Copy link
Copy Markdown
Member

We changed bounds cast expressions to take a bounds expression as the
second argument, instead of implicitly inferring the kind of bounds
expression from the number of arguments. This is more verbose but
clearer. Bounds cast expressions now have two forms:

Op(e1)
Op(e1, bounds-expression)

where Op is one of _Assume_bounds_cast or _Dynamic_bounds_cast
and T must be a pointer type.

This change also improves the error messages and renames the
error message names to be clearer.

Testing:

  • There will be a corresponding Checked C repo change to tests
    for bounds cast expressions.
  • Add a test for error recovery from bounds cast expression syntax
    errors.
  • Passed local testing on x64 Windows and automated testing on Linux.

We changed bounds cast expressions to take a bounds expression as the
second argument, instead of implicitly inferring the type of bounds
expression from the number of arguments.  This is more verbose but
clearer.  Bounds cast expressions now have two forms:

Op<T>(e1)
Op<T>(e1, bounds-expression)

where Op is one of _Assume_bounds_cast or _Dynamic_bounds_cast
and T must be a pointer type.

This change also improves the error messages and renames the
error message names to be clearer.

Testing:
- There will be a corresponding Checked C repo change to tests
  for bounds cast expressions.
- Add a test for error recovery from bounds cast expression syntax
  errors.
@dtarditi dtarditi requested a review from awruef March 21, 2018 17:36

@awruef awruef left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Looks good, just two comments about the same thing with GetTypeFromParser.

Comment thread lib/Sema/SemaExpr.cpp

QualType DestTy = GetTypeFromParser(D, &castTInfo);
SourceLocation TypeLoc = (castTInfo->getTypeLoc()).getBeginLoc();
QualType DestTy = GetTypeFromParser(D, &CastTInfo);

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

I looked at the code for GetTypeFromParser and I think that either way, it will set CastTInfo to nullptr, so it's okay that it's declared un-initialized. Maybe there should be an assert before it's de-referenced, though?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Good catch. We should check that CastTInfo is non-null and use a different source location if it is. I'll use the location of the '<' symbol given an expression of the form Op<T>(...), where Op is a bounds cast operator. It'll point the programmer in the right direction, although we could be off by any white space between the '<' and T.

Comment thread lib/Sema/SemaExpr.cpp
QualType DestTy = GetTypeFromParser(D, &castTInfo);
ExprResult ParsedBounds = CorrectDelayedTyposInExpr(bounds);
SourceLocation TypeLoc = (castTInfo->getTypeLoc()).getBeginLoc();
QualType DestTy = GetTypeFromParser(D, &CastTInfo);

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

I looked at the code for GetTypeFromParser and I think that either way, it will set CastTInfo to nullptr, so it's okay that it's declared un-initialized. Maybe there should be an assert before it's de-referenced, though?

@dtarditi dtarditi merged commit e4503f3 into master Mar 22, 2018
awruef pushed a commit to awruef/checkedc-clang that referenced this pull request Apr 4, 2018
…dc#468)

We changed bounds cast expressions to take a bounds expression as the
second argument, instead of implicitly inferring the kind of bounds
expression from the number of arguments.  This is more verbose but
clearer.  Bounds cast expressions now have two forms:

Op<T>(e1)
Op<T>(e1, bounds-expression)

where Op is one of _Assume_bounds_cast or _Dynamic_bounds_cast
and T must be a pointer type.

This change also improves the error messages and renames the
error message names to be clearer.

Testing:
- There is corresponding Checked C repo change to tests
  for bounds cast expressions.
- Add a test for error recovery from bounds cast expression syntax
  errors.
- Passed local testing on x64 Windows and automated testing on Linux.
@dtarditi dtarditi deleted the issue448 branch April 11, 2018 21:02
sulekhark pushed a commit that referenced this pull request Jul 8, 2021
* Fixed iss468

* Added test

* Fixing review issues
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.

2 participants