r203299 - In my tests, I'm finding that declaring iterators in terms of ranges can sometimes have dangerous side-effects where the range temporary is destroyed, taking the underlying iterators out with it.
David Blaikie
dblaikie at gmail.com
Fri Mar 7 14:28:35 PST 2014
On Fri, Mar 7, 2014 at 2:17 PM, Aaron Ballman <aaron at aaronballman.com> wrote:
> Author: aaronballman
> Date: Fri Mar 7 16:17:20 2014
> New Revision: 203299
>
> URL: http://llvm.org/viewvc/llvm-project?rev=203299&view=rev
> Log:
> In my tests, I'm finding that declaring iterators in terms of ranges can sometimes have dangerous side-effects where the range temporary is destroyed, taking the underlying iterators out with it.
Um... what? do you have a concrete example because that doesn't sound
right at all, at least not for iterator_range<T> which just holds
copies of the iterator - are some of our iterators non-copyable (or
not safely/sanely copyable in some way)? All the begin/end's return by
value, so I don't see how they could be related to the copy that
happens to be in the range temporary...
>
> This changes the iterators so that they are no longer implemented in terms of ranges (so it's a very partial revert of the existing rangification efforts).
>
> Modified:
> cfe/trunk/include/clang/AST/Decl.h
> cfe/trunk/include/clang/AST/DeclBase.h
> cfe/trunk/include/clang/AST/DeclObjC.h
> cfe/trunk/include/clang/AST/Redeclarable.h
> cfe/trunk/lib/AST/DeclBase.cpp
>
> Modified: cfe/trunk/include/clang/AST/Decl.h
> URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/AST/Decl.h?rev=203299&r1=203298&r2=203299&view=diff
> ==============================================================================
> --- cfe/trunk/include/clang/AST/Decl.h (original)
> +++ cfe/trunk/include/clang/AST/Decl.h Fri Mar 7 16:17:20 2014
> @@ -1846,14 +1846,20 @@ public:
> typedef llvm::iterator_range<param_iterator> param_range;
> typedef llvm::iterator_range<param_const_iterator> param_const_range;
>
> - param_iterator param_begin() { return params().begin(); }
> - param_iterator param_end() { return params().end(); }
> + param_iterator param_begin() { return param_iterator(ParamInfo); }
> + param_iterator param_end() {
> + return param_iterator(ParamInfo + param_size());
> + }
> param_range params() {
> return param_range(ParamInfo, ParamInfo + param_size());
> }
>
> - param_const_iterator param_begin() const { return params().begin(); }
> - param_const_iterator param_end() const { return params().end(); }
> + param_const_iterator param_begin() const {
> + return param_const_iterator(ParamInfo);
> + }
> + param_const_iterator param_end() const {
> + return param_const_iterator(ParamInfo + param_size());
> + }
> param_const_range params() const {
> return param_const_range(ParamInfo, ParamInfo + param_size());
> }
>
> Modified: cfe/trunk/include/clang/AST/DeclBase.h
> URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/AST/DeclBase.h?rev=203299&r1=203298&r2=203299&view=diff
> ==============================================================================
> --- cfe/trunk/include/clang/AST/DeclBase.h (original)
> +++ cfe/trunk/include/clang/AST/DeclBase.h Fri Mar 7 16:17:20 2014
> @@ -780,8 +780,10 @@ public:
> redecl_iterator());
> }
>
> - redecl_iterator redecls_begin() const { return redecls().begin(); }
> - redecl_iterator redecls_end() const { return redecls().end(); }
> + redecl_iterator redecls_begin() const {
> + return redecl_iterator(const_cast<Decl *>(this));
> + }
> + redecl_iterator redecls_end() const { return redecl_iterator(); }
>
> /// \brief Retrieve the previous declaration that declares the same entity
> /// as this declaration, or NULL if there is no previous declaration.
> @@ -1311,16 +1313,16 @@ public:
> /// decls_begin/decls_end - Iterate over the declarations stored in
> /// this context.
> decl_range decls() const;
> - decl_iterator decls_begin() const { return decls().begin(); }
> - decl_iterator decls_end() const { return decls().end(); }
> + decl_iterator decls_begin() const;
> + decl_iterator decls_end() const { return decl_iterator(); }
> bool decls_empty() const;
>
> /// noload_decls_begin/end - Iterate over the declarations stored in this
> /// context that are currently loaded; don't attempt to retrieve anything
> /// from an external source.
> decl_range noload_decls() const;
> - decl_iterator noload_decls_begin() const { return noload_decls().begin(); }
> - decl_iterator noload_decls_end() const { return noload_decls().end(); }
> + decl_iterator noload_decls_begin() const;
> + decl_iterator noload_decls_end() const { return decl_iterator(); }
>
> /// specific_decl_iterator - Iterates over a subrange of
> /// declarations stored in a DeclContext, providing only those that
>
> Modified: cfe/trunk/include/clang/AST/DeclObjC.h
> URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/AST/DeclObjC.h?rev=203299&r1=203298&r2=203299&view=diff
> ==============================================================================
> --- cfe/trunk/include/clang/AST/DeclObjC.h (original)
> +++ cfe/trunk/include/clang/AST/DeclObjC.h Fri Mar 7 16:17:20 2014
> @@ -352,10 +352,14 @@ public:
> return param_const_range(getParams(), getParams() + NumParams);
> }
>
> - param_const_iterator param_begin() const { return params().begin(); }
> - param_const_iterator param_end() const { return params().end(); }
> - param_iterator param_begin() { return params().begin(); }
> - param_iterator param_end() { return params().end(); }
> + param_const_iterator param_begin() const {
> + return param_const_iterator(getParams());
> + }
> + param_const_iterator param_end() const {
> + return param_const_iterator(getParams() + NumParams);
> + }
> + param_iterator param_begin() { return param_iterator(getParams()); }
> + param_iterator param_end() { return param_iterator(getParams() + NumParams); }
>
> // This method returns and of the parameters which are part of the selector
> // name mangling requirements.
>
> Modified: cfe/trunk/include/clang/AST/Redeclarable.h
> URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/AST/Redeclarable.h?rev=203299&r1=203298&r2=203299&view=diff
> ==============================================================================
> --- cfe/trunk/include/clang/AST/Redeclarable.h (original)
> +++ cfe/trunk/include/clang/AST/Redeclarable.h Fri Mar 7 16:17:20 2014
> @@ -171,8 +171,11 @@ public:
> redecl_iterator());
> }
>
> - redecl_iterator redecls_begin() const { return redecls().begin(); }
> - redecl_iterator redecls_end() const { return redecls().end(); }
> + redecl_iterator redecls_begin() const {
> + return redecl_iterator(
> + const_cast<decl_type *>(static_cast<const decl_type *>(this)));
> + }
> + redecl_iterator redecls_end() const { return redecl_iterator(); }
>
> friend class ASTDeclReader;
> friend class ASTDeclWriter;
>
> Modified: cfe/trunk/lib/AST/DeclBase.cpp
> URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/AST/DeclBase.cpp?rev=203299&r1=203298&r2=203299&view=diff
> ==============================================================================
> --- cfe/trunk/lib/AST/DeclBase.cpp (original)
> +++ cfe/trunk/lib/AST/DeclBase.cpp Fri Mar 7 16:17:20 2014
> @@ -1080,12 +1080,22 @@ DeclContext::decl_range DeclContext::nol
> return decl_range(decl_iterator(FirstDecl), decl_iterator());
> }
>
> +DeclContext::decl_iterator DeclContext::noload_decls_begin() const {
> + return decl_iterator(FirstDecl);
> +}
> +
> DeclContext::decl_range DeclContext::decls() const {
> if (hasExternalLexicalStorage())
> LoadLexicalDeclsFromExternalStorage();
> return decl_range(decl_iterator(FirstDecl), decl_iterator());
> }
>
> +DeclContext::decl_iterator DeclContext::decls_begin() const {
> + if (hasExternalLexicalStorage())
> + LoadLexicalDeclsFromExternalStorage();
> + return decl_iterator(FirstDecl);
> +}
> +
> bool DeclContext::decls_empty() const {
> if (hasExternalLexicalStorage())
> LoadLexicalDeclsFromExternalStorage();
>
>
> _______________________________________________
> cfe-commits mailing list
> cfe-commits at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
More information about the cfe-commits
mailing list