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. 2. Listeners may not be called at all, due to the shortcutting || in visitInputFile. 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?
commit f928593f5a039cd449f7c8c5ae3a94eb2b5f5975 Author: Justin Bogner <[email protected]> Date: Mon May 12 22:48:20 2014 -0700 Frontend: Propagate ASTReaderListener API in ChainedASTReaderListener ASTReaderListener's documentation states that visitInputFile will be called based on the return values of needsInputFileVisitation and needsSystemInputFileVisitation, but ChainedASTReaderListener may call these methods on a child listener based on the values returned by the other child. Even worse, the calls to visitInputFile may be short-circuited due to the use of the boolean or, so the calls to visit may not occur at all for the second listener. This updates ChainedASTReaderListener::visitInputFile to propagate the ASTReaderListener behaviour to both children. diff --git a/lib/Serialization/ASTReader.cpp b/lib/Serialization/ASTReader.cpp index f6d705a..57a96ab 100644 --- a/lib/Serialization/ASTReader.cpp +++ b/lib/Serialization/ASTReader.cpp @@ -135,8 +135,14 @@ void ChainedASTReaderListener::visitModuleFile(StringRef Filename) { bool ChainedASTReaderListener::visitInputFile(StringRef Filename, bool isSystem, bool isOverridden) { - return First->visitInputFile(Filename, isSystem, isOverridden) || - Second->visitInputFile(Filename, isSystem, isOverridden); + bool Continue = true; + if (First->needsInputFileVisitation() && + (!isSystem || First->needsSystemInputFileVisitation())) + Continue |= First->visitInputFile(Filename, isSystem, isOverridden); + if (Second->needsInputFileVisitation() && + (!isSystem || Second->needsSystemInputFileVisitation())) + Continue |= Second->visitInputFile(Filename, isSystem, isOverridden); + return Continue; } //===----------------------------------------------------------------------===//
_______________________________________________ cfe-commits mailing list [email protected] http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
