[PATCH] D60381: FileCheck [1/12]: Move variable table in new object

Fri Apr 12 01:02:07 PDT 2019

arsenm added inline comments.

================
Comment at: llvm/include/llvm/Support/FileCheck.h:138
+  /// variables at the start of any given CHECK line.
+  FileCheckPatternContext *Context;
+
----------------
thopre wrote:
> thopre wrote:
> > arsenm wrote:
> > > thopre wrote:
> > > > jhenderson wrote:
> > > > > Just to be clear, this is shared between all patterns?
> > > > Yes, updated the comment to make it clear.
> > > Why is this shared_ptr? For a context, there should be clear ownership. Each pattern doesn't need to have a potentially owning reference. It can live somewhere else
> > Hi Matt, thanks for the review!
> > 
> > The object is created in readCheckFile function and cease to be used in checkInput without a pointer being passed around explicitely. I used to delete the pointer taken from one of the pattern but James suggested to use a shared_ptr. It is overkill but allow the code to still work in case it is changed slightly. If the patterns hold a regular pointer I don't know in what shared_ptr to store the ownership and would need to revert to the initial approach.
> > 
> > Is there any other solution I've missed?
> How about creating a pointer to the context in the FileCheck class? I'm tempted to make it a regular pointer since it is clear it can be destroyed once checkInput is finished but let me know if I should be using a shared_ptr instead.
Yes, that's what I was thinking. Each instance needs access to the context, but it shouldn't potentially own it.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D60381