Skip to content

Conversation

@bricknerb
Copy link
Contributor

@bricknerb bricknerb commented Mar 10, 2025

This is a followup of a comment in #5062.

Add a mutable AST pointer to FileContext.

This is necessary since we use Clang with lack of const correctness.

Alternatives in Clang:

Alternatives in Carbon:

  • Use const_cast on ASTContext when calling createMangleContext().
  • Make FileContext::sem_ir_ point to a mutable SemIR::File.
  • Change File::cpp_ast() to be const while keeping it return a mutable pointer.

Part of #4666.

This is a followup of [a comment](https://github.com/carbon-language/carbon-lang/pull/5062/files/89e56d51858bcc18d4242d4e5c9ee0e7496d887e#r1979993815) in carbon-language#5062.

Change `File::cpp_ast()` to be `const` while keeping it returning a non-const pointer to `clang::ASTUnit`.
This makes the fact we use [Clang with lack of const correctness](llvm/llvm-project#130096 (comment)) explicit.

Alternatives:
* Change `ASTUnit::getASTContext() const` to return a non-const `ASTContext`. [Rejected due to weakening const correctness](llvm/llvm-project#130096).
* Change `createMangleContext()` to be `const`. Tried that and it seems like it relies heavily on non const API.
* Change Clang `MangleContext::mangleName()` to `const`.
* Use `const_cast` on `ASTContext` when calling `createMangleContext()`.
@github-actions github-actions bot requested a review from josh11b March 10, 2025 14:41
@jonmeow
Copy link
Contributor

jonmeow commented Mar 10, 2025

I share the same concerns regarding a const accessor returning a non-const value as the clang owners gave you. If it were how the API were intended to behave, that'd be one thing, but I don't think we should work around that in our code.

Going on the advice from clang owners, you could const_cast with a TODO. But maybe it would make more sense to pass the mutable AST straight to lowering, rather than trying to pull it off the const SemIR?

@bricknerb
Copy link
Contributor Author

I share the same concerns regarding a const accessor returning a non-const value as the clang owners gave you. If it were how the API were intended to behave, that'd be one thing, but I don't think we should work around that in our code.

Going on the advice from clang owners, you could const_cast with a TODO. But maybe it would make more sense to pass the mutable AST straight to lowering, rather than trying to pull it off the const SemIR?

ok, SG, passing a mutable AST to lowering now.

@bricknerb bricknerb requested a review from jonmeow March 11, 2025 12:25
@jonmeow jonmeow added this pull request to the merge queue Mar 12, 2025
Merged via the queue into carbon-language:trunk with commit a4a229b Mar 12, 2025
8 checks passed
@bricknerb bricknerb deleted the const branch March 17, 2025 12:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants