Details
- Reviewers
aaron.ballman tahonermann
Diff Detail
Event Timeline
| clang/include/clang/AST/Stmt.h | ||
|---|---|---|
| 2606–2607 ↗ | (On Diff #552230) | I don't think this initialization is necessary. The constructor for ForStmt initializes all of the valid elements: https://github.com/llvm/llvm-project/blob/87c5a3e203ae3643bc13c9a13917b92a657f4358/clang/lib/AST/Stmt.cpp#L1024 The EmptyShell constructor does not initialize anything but that's because it is piecemeal initialized by the AST importer, but all of its fields are also initialized: https://github.com/llvm/llvm-project/blob/87c5a3e203ae3643bc13c9a13917b92a657f4358/clang/lib/Serialization/ASTReaderStmt.cpp#L296 |
| llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp | ||
| 934 | This is necessary to initialize because emitTargetKernel() has an early return which does not initialize the passed reference to this object. | |
| clang/include/clang/AST/Stmt.h | ||
|---|---|---|
| 2606–2607 ↗ | (On Diff #552230) | Declarations like this are common in the AST. I'm curious while static analysis flagged this one in particular. Perhaps it identified a path where one or more of the elements don't get initialized? If so, this would be a good find and a fix should be applied to that path. |
| clang/include/clang/AST/Stmt.h | ||
|---|---|---|
| 2606–2607 ↗ | (On Diff #552230) | I will follow-up with this one more and open a different PR if necessary. Removing this change. |
| 2606–2607 ↗ | (On Diff #552230) | Thank you for this analysis! |
Looks good to me.
| llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp | ||
|---|---|---|
| 934 | Return is passed to Builder.CreateIsNotNull(). I briefly inspected and it looks like it handles a null argument, so I agree this looks like the right fix. | |
This is necessary to initialize because emitTargetKernel() has an early return which does not initialize the passed reference to this object.