[PATCH] D126944: [Clang] Fix memory leak due to TemplateArgumentListInfo used in AST node.

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman accepted this revision.
aaron.ballman added a comment.
This revision is now accepted and ready to land.

LGTM; I think it's reasonable to not have test coverage specific to the changes here (the existing test coverage already uncovered this issue). It might be worth mentioning that the leak was fixed in a release note, however. Also, I did find one naming convention issue that should be fixed before landing.

Thanks for fixing this up!



================
Comment at: clang/include/clang/AST/DeclTemplate.h:2778-2779
 
-  const TemplateArgumentListInfo &getTemplateArgsInfo() const {
+  const ASTTemplateArgumentListInfo *getTemplateArgsInfo() const {
     return TemplateArgsInfo;
   }
----------------
browneee wrote:
> aaron.ballman wrote:
> > Do we want to assert that `TemplateArgsInfo` is nonnull and return a const reference instead?
> > 
> > Basically, I'm wondering if we can remove a bunch of the pointers and go with references as much as possible; it seems to me that a null pointer is a sign of an error, same as with `TemplateArgs`.
> This may be why https://reviews.llvm.org/D126937 is failing tests.
> 
> My goals is to fix the memory leak, so the sanitizer buildbots are clean. I would agree there is probably more cleanup to do for this member variable after https://reviews.llvm.org/D126944 - but fixing the memory leak is the urgent thing for me.
Okay, I'm fine not handling the extra safety measures at the moment.


================
Comment at: clang/lib/Sema/SemaTemplateInstantiateDecl.cpp:5563-5565
+      for (const TemplateArgumentLoc& arg : ArgInfo->arguments()) {
+        TemplateArgInfo.addArgument(arg);
+      }
----------------
aaron.ballman wrote:
> More coding standard changes -- btw, it looks like you need to run the patch through clang-format (https://clang.llvm.org/docs/ClangFormat.html#script-for-patch-reformatting).
It looks like you may have missed the rename suggestion of switching `arg` to `Arg` per our usual coding conventions. You can fix that when landing though.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D126944/new/

https://reviews.llvm.org/D126944