On May 21, 2014, at 7:32 PM, Ben Langmuir <[email protected]> wrote:
> On May 21, 2014, at 6:56 PM, Justin Bogner <[email protected]> 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> > _______________________________________________ cfe-commits mailing list [email protected] http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
