Closed GoogleCodeExporter closed 8 years ago
GoogleCodeExporter
commented
8 years ago Just to clarify, this change doesn't happen in r151489, but that is the
revision I tested against.Original comment by peter.wa...@gmail.com on 27 Feb 2012 at 6:48
GoogleCodeExporter
commented
8 years ago This seems related to r150682 ->
http://www.mail-archive.com/cfe-commits@cs.uiuc.edu/msg46100.html
(which seems to give a way to workaround the evolution)
HTH.Original comment by luc.herm...@gmail.com on 1 Mar 2012 at 5:54
GoogleCodeExporter
commented
8 years ago The attached patch fixes it for me on clang/llvm r151926.
I have added a local function getConstructor which might be overkill/ not fit
with the styleguide ...
In wyu_ast_util.cc GetTplTypeResugarMapForFunction() I removed the special
handling for NewExpr because it seems to have moved to ConstructorExpr
completely. Original comment by FFabioFr...@googlemail.com on 4 Mar 2012 at 6:31
Attachments:
GoogleCodeExporter
commented
8 years ago Fabio, thanks for your patch. I'm no longer really working on iwyu, so am not
noticing when the code regresses; thanks for keeping a look-out.
Your patch looks fine. My only suggestions are stylistic: rename
getConstructor to GetConstructor, and put it in iwyu-ast-util, which seems to
be its logical home. (No reason to limit its use to just the one function it's
used in now, I don't think.)
Once you've done that, feel free to commit. I will need to give you commit
access. Is the gmx address the right one to add, or would you prefer another?Original comment by csilv...@gmail.com on 6 Mar 2012 at 11:11
GoogleCodeExporter
commented
8 years ago Ok, done. Yes the gmx address is fine.Original comment by FFabioFr...@googlemail.com on 7 Mar 2012 at 10:20
Attachments:
GoogleCodeExporter
commented
8 years ago Looks good. Only comment: I'm surprised this line compiles:
} CXXConstructorDecl * GetConstructor(clang::CXXNewExpr* expr);
Doesn't it need to be
clang::CXXConstructorDecl
?
Also, one style nit: the style in the .h file seems to be to put the * to the
left:
CXXConstructorDecl* GetConstructor(clang::CXXNewExpr* expr);
Might as well do it that way.
You should have commit access now! Let me know if you have any trouble.Original comment by csilv...@gmail.com on 7 Mar 2012 at 11:13
GoogleCodeExporter
commented
8 years ago [deleted comment]I applied the patch on revision 152413 but get among others the following
compile errors:
In file included from
/home/gerben/src/llvm/tools/clang/tools/include-what-you-use/iwyu_cache.cc:15:
/home/gerben/src/llvm/tools/clang/tools/include-what-you-use/iwyu_ast_util.h:789
: error: expected constructor, destructor, or type conversion before ‘*’
token
In file included from
/home/gerben/src/llvm/tools/clang/tools/include-what-you-use/iwyu_location_util.
cc:12:
/home/gerben/src/llvm/tools/clang/tools/include-what-you-use/iwyu_ast_util.h:789
:
...
/home/gerben/src/llvm/tools/clang/tools/include-what-you-use/iwyu.cc: In member
function ‘bool
include_what_you_use::BaseAstVisitor<Derived>::TraverseCXXNewExpr(clang::CXXNewE
xpr*)’:
/home/gerben/src/llvm/tools/clang/tools/include-what-you-use/iwyu.cc:898:
error: there are no arguments to ‘GetConstructor’ that depend on a template
parameter, so a declaration of ‘GetConstructor’ must be available
/home/gerben/src/llvm/tools/clang/tools/include-what-you-use/iwyu.cc:898: note:
(if you use ‘-fpermissive’, G++ will accept your code, but allowing the use
of an undeclared name is deprecated)
make[4]: ***
[/home/gerben/src/build-llvm/tools/clang/tools/include-what-you-use/Release/iwyu
_ast_util.o] Error 1
...Original comment by gerben.o...@gmail.com on 9 Mar 2012 at 5:08
Attachments:
GoogleCodeExporter
commented
8 years ago I'm sorry the last patch was faulty, this one should work. It compiles and
passes all tests except badinc.
I did not recieve any credentials for the svn yet.Original comment by FFabioFr...@googlemail.com on 10 Mar 2012 at 1:52
Attachments:
GoogleCodeExporter
commented
8 years ago Ok never used google code before, svn commit works now, fixed in r347Original comment by FFabioFr...@googlemail.com on 10 Mar 2012 at 2:07
GoogleCodeExporter
commented
8 years ago Glad you got it checked in -- thanks for fixing things up!
The goal is that all tests should pass, including badinc. What is the failure?
Do you have a fix that looks right?Original comment by csilv...@gmail.com on 10 Mar 2012 at 10:31
GoogleCodeExporter
commented
8 years ago not yet. And I got a new linking failure since I updated clang today, so not
realy sure how to proceed.
I also only just started looking into iwyu (and clang) in depth just now, so it
will probably take me a bit. here is the error log ...
======================================================================
FAIL: runTest (__main__.badinc)
----------------------------------------------------------------------
Traceback (most recent call last):
File "./run_iwyu_tests.py", line 112, in <lambda>
{'runTest': lambda self, f=filename: self.RunOneTest(f)})
File "./run_iwyu_tests.py", line 88, in RunOneTest
iwyu_flags, verbose=True)
File "/Users/fabio/Development/LLVM/llvm/tools/clang/tools/include-what-you-use/iwyu_test_util.py", line 401, in TestIwyuOnRelativeFile
test_case.assertTrue(not failures, ''.join(failures))
AssertionError:
tests/badinc.cc:547: Unexpected diagnostic:
I2_TemplateClass::~I2_TemplateClass<FOO> is defined in "tests/badinc-i2-inl.h",
which isn't directly #included.
tests/badinc.cc:1136: Unexpected diagnostic:
errno is defined in <sys/errno.h>, which isn't directly #included.
tests/badinc.cc:1616: Unexpected diagnostic:
I2_Enum is defined in "tests/badinc-i2.h", which isn't directly #included.
tests/badinc.cc:1637: Unexpected diagnostic:
I2_Class::~I2_Class is defined in "tests/badinc-i2-inl.h", which isn't directly
#included.
tests/badinc.cc:1648: Unexpected diagnostic:
I2_Class::~I2_Class is defined in "tests/badinc-i2-inl.h", which isn't directly
#included.
tests/badinc.cc:1657: Unexpected diagnostic:
I1_Struct is defined in "tests/badinc-i1.h", which isn't directly #included.
tests/badinc.cc:1657: Unexpected diagnostic:
I2_Class::~I2_Class is defined in "tests/badinc-i2-inl.h", which isn't directly
#included.
tests/badinc.cc:1675: Unexpected diagnostic:
I2_Class::~I2_Class is defined in "tests/badinc-i2-inl.h", which isn't directly
#included.
tests/badinc.cc:1706: Unexpected diagnostic:
I2_Class::~I2_Class is defined in "tests/badinc-i2-inl.h", which isn't directly
#included.
tests/badinc.h:133: Unexpected diagnostic:
errno is defined in <sys/errno.h>, which isn't directly #included.
Unexpected summary diffs for tests/badinc.h:
+++
@@ -1,5 +1,6 @@
tests/badinc.h should add these lines:
#include <stdio.h>
+#include <sys/errno.h>
#include <set>
#include <utility>
#include <vector>
@@ -8,14 +9,15 @@
tests/badinc.h should remove these lines:
- #include <ctype.h> // lines XX-XX
+- #include <errno.h> // lines XX-XX
- #include <math.h> // lines XX-XX
- #include "tests/badinc-d2.h" // lines XX-XX
- class H_ForwardDeclareClass; // lines XX-XX
- template <typename T> class I2_TypedefOnly_Class; // lines XX-XX
The full include-list for tests/badinc.h:
-#include <errno.h> // for errno
#include <stdio.h> // for NULL, printf
+#include <sys/errno.h> // for errno
#include <queue> // for queue
#include <set> // for set
#include <string> // for string
---
----------------------------------------------------------------------
Ran 61 tests in 3.848s
FAILED (failures=1)
Original comment by FFabioFr...@googlemail.com on 10 Mar 2012 at 10:50
GoogleCodeExporter
commented
8 years ago [deleted comment]Looks like issue #22 Test badinc fails on mac @ r62. Are you using Mac OS X?Original comment by vsap...@gmail.com on 10 Mar 2012 at 11:01
GoogleCodeExporter
commented
8 years ago Yes, I use Mac OS X. Will have a look.Original comment by FFabioFr...@googlemail.com on 10 Mar 2012 at 11:03
GoogleCodeExporter
commented
8 years ago I just tested rev 347 on llvm, compiler-rt, clang rev's 152621 but got the
following errors on 'make ENABLE_OPTIMIZED=1 DISABLE_ASSERTIONS=1 -j6':
llvm[4]: Linking Release executable include-what-you-use (without symbols)
Release/lib/libclangFrontend.a(DiagnosticRenderer.o): In function `(anonymous
namespace)::FixitReceiver::~FixitReceiver()':
DiagnosticRenderer.cpp:(.text+0xed): undefined reference to `vtable for
clang::edit::EditsReceiver'
Release/lib/libclangFrontend.a(DiagnosticRenderer.o): In function
`clang::DiagnosticRenderer::emitDiagnostic(clang::SourceLocation,
clang::DiagnosticsEngine::Level, llvm::StringRef,
llvm::ArrayRef<clang::CharSourceRange>, llvm::ArrayRef<clang::FixItHint>,
llvm::PointerUnion<clang::Diagnostic const*, clang::StoredDiagnostic const*>)':
DiagnosticRenderer.cpp:(.text+0x1bb4): undefined reference to
`clang::edit::Commit::insertFromRange(clang::SourceLocation,
clang::CharSourceRange, bool, bool)'
DiagnosticRenderer.cpp:(.text+0x1c1d): undefined reference to
`clang::edit::Commit::replace(clang::CharSourceRange, llvm::StringRef)'
DiagnosticRenderer.cpp:(.text+0x1cdd): undefined reference to
`clang::edit::EditedSource::commit(clang::edit::Commit const&)'
DiagnosticRenderer.cpp:(.text+0x1e65): undefined reference to
`clang::edit::Commit::remove(clang::CharSourceRange)'
DiagnosticRenderer.cpp:(.text+0x1e92): undefined reference to
`clang::edit::Commit::insert(clang::SourceLocation, llvm::StringRef, bool,
bool)'
DiagnosticRenderer.cpp:(.text+0x1ec9): undefined reference to
`clang::edit::EditedSource::applyRewrites(clang::edit::EditsReceiver&)'
DiagnosticRenderer.cpp:(.text+0x1ecf): undefined reference to `vtable for
clang::edit::EditsReceiver'
Release/lib/libclangFrontend.a(DiagnosticRenderer.o):(.data.rel.ro+0x18):
undefined reference to
`clang::edit::EditsReceiver::remove(clang::CharSourceRange)'
/home/gerben/src/build-llvm/Release/lib/libclangSema.a(SemaExprObjC.o): In
function `T.4424':
SemaExprObjC.cpp:(.text+0x2cf0): undefined reference to
`clang::edit::rewriteObjCRedundantCallWithLiteral(clang::ObjCMessageExpr
const*, clang::NSAPI const&, clang::edit::Commit&)'
SemaExprObjC.cpp:(.text+0x2e92): undefined reference to
`clang::edit::Commit::Edit::getInsertFromRange(clang::SourceManager&) const'
SemaExprObjC.cpp:(.text+0x2f93): undefined reference to
`clang::edit::Commit::Edit::getFileRange(clang::SourceManager&) const'
collect2: ld returned 1 exit status
These occur after llvm[4]: Compiling iwyu_... for Release build
Original comment by gerben.o...@gmail.com on 13 Mar 2012 at 10:20
Attachments:
GoogleCodeExporter
commented
8 years ago Yes, I am seeing this, too. Can you report it as a new issue?Original comment by FFabioFr...@googlemail.com on 13 Mar 2012 at 10:24
GoogleCodeExporter
commented
8 years ago Attached patch fixes the destructor issue:
AssertionError:
tests/badinc.cc:547: Unexpected diagnostic:
I2_TemplateClass::~I2_TemplateClass<FOO> is defined in "tests/badinc-i2-inl.h", which
isn't directly #included.
This seems to be the only remaining problem with the CXXNewExpr handling, as
we've discussed on the list.
Original comment by kim.gras...@gmail.com on 5 Aug 2012 at 3:04
Attachments:
GoogleCodeExporter
commented
8 years ago Thanks for patch, Kim. Based on it I've done a few more changes.
In Clang r150682 was changed how CXXConstructorDecl is handled in CXXNewExpr -
instead of storing it separately, now it is stored as one of subexpressions and
that's why it is traversed and visited automatically. That's why we don't need
to handle constructor call in TraverseCXXNewExpr(), it will be handled in
TraverseCXXConstructExpr(). Also change in traversal caused unnecessary
TraverseImplicitDestructorCall(), which is now avoided if CXXConstructExpr is
in CXXNewExpr. Also I've removed unnecessary function and cleaned up style a
little bit.
Kim, can you review this patch?Original comment by vsap...@gmail.com on 19 Aug 2012 at 6:59
Attachments:
GoogleCodeExporter
commented
8 years ago All tests (62 of them) pass on Linux.
If I understand this correctly, you no longer handle CXXNewExpr, except for
overloaded operator new? I had that thought as well to get rid of the reports
that only differ in column #. Glad it seems to work!
And I like IsCXXConstructExprInNewExpr -- the GetConstructor method looked a
little atypical earlier.
It took me a while to see what you changed in VisitCXXNewExpr, but I like the
style changes. They're so small they might as well go in the same commit.
All in all, looks good, please commit!Original comment by kim.gras...@gmail.com on 19 Aug 2012 at 7:54
GoogleCodeExporter
commented
8 years ago Committed r371.Original comment by vsap...@gmail.com on 2 Sep 2012 at 5:51
Original issue reported on code.google.com by
peter.wa...@gmail.comon 27 Feb 2012 at 6:47