[PATCH] Propagate ASTReaderListener API in ChainedASTReaderListener

[Argyrios Kyrtzidis]

On May 21, 2014, at 7:32 PM, Ben Langmuir <blangmuir at apple.com> wrote:

> On May 21, 2014, at 6:56 PM, Justin Bogner <mail at justinbogner.com> wrote:
> 
>> I ran into some issues in ChainedASTReaderListener while trying to use
>> ASTReader::addListener. When there are multiple listeners, a couple of
>> problems can (and do!) occur:
>> 
>> 1. Listeners can be called for file types they don't expect (ie, system
>>  files when they return false for needsSystemInputFileVisitation),
>>  leading to asserts and other bad behaviour.
> 
> Yep, good catch.
> 
>> 
>> 2. Listeners may not be called at all, due to the shortcutting || in
>>  visitInputFile.
> 
> D’oh.
> 
> Argyrios, it doesn’t look like any in-tree users of this API ever return false.  Do you think we can remove the bool return type and just always continue?  This doesn’t need to be done in this patch by any means.

Yes, or we could make ChainedASTReaderListener more complex but it doesn't seem to be worth it.

> 
>> +  bool Continue = true;
>> +  if (First->needsInputFileVisitation() &&
>> 
> 
> 
> The above notwithstanding, Continue should start false, or we will never return true :-)  If you fix that, then LGTM!
> 
> Ben
> 
>> 
>> The attached patch fixes this by adding checks to and removing
>> shortcutting from ChainedASTReaderListener::visitInputFile. We could,
>> alternatively, avoid the extra checks, update the docs for
>> ASTReaderListener to say that visitInputFile may be called for file
>> types the client didn't ask for, and update all clients, but that seems
>> like a worse approach.
>> 
>> Okay to commit?
>> 
>> <chained-reader-listener.patch>
>