Re: [PATCH] D23279: clang-reorder-fields

This revision was automatically updated to reflect the committed changes. Closed by commit rL280431: Add clang-reorder-fields to clang-tools-extra (authored by alexshap). Changed prior to commit: https://reviews.llvm.org/D23279?vs=69814&id=70102#toc Repository: rL LLVM https://reviews.llvm.

Re: [PATCH] D23279: clang-reorder-fields

djasper added a comment. Looks good :). Repository: rL LLVM https://reviews.llvm.org/D23279 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] D23279: clang-reorder-fields

alexshap marked an inline comment as done. alexshap added a comment. Repository: rL LLVM https://reviews.llvm.org/D23279 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] D23279: clang-reorder-fields

alexshap marked an inline comment as done. alexshap added a comment. Repository: rL LLVM https://reviews.llvm.org/D23279 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] D23279: clang-reorder-fields

alexshap updated this revision to Diff 69814. alexshap added a comment. 1. Fix comments (hope so). 2. if (const auto *SyntacticForm = InitListEx->getSyntacticForm()) InitListEx = SyntacticForm; Repository: rL LLVM https://reviews.llvm.org/D23279 Files: CMakeLists.txt clang-reorder-field

Re: [PATCH] D23279: clang-reorder-fields

djasper added inline comments. Comment at: clang-reorder-fields/ReorderFieldsAction.cpp:181 @@ +180,3 @@ + assert(InitListEx && "Init list expression is null"); + // we care only about brace initializations + // which have valid source locations Write full sent

Re: [PATCH] D23279: clang-reorder-fields

alexshap updated this revision to Diff 69807. alexshap added a comment. 1. Fix handling of initListExprs. http://clang.llvm.org/doxygen/Expr_8h_source.html#l03701 2. Fix handling of unchanged files 3. Add tests (AggregatePartialInitialization and ClassDifferentFieldsAccesses) Repository: rL

Re: [PATCH] D23279: clang-reorder-fields

alexshap added a comment. good call, i have found a bug in handling of initListExprs. Will update this diff and add tests. Repository: rL LLVM https://reviews.llvm.org/D23279 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.l

Re: [PATCH] D23279: clang-reorder-fields

djasper added a comment. As per my comment, please add tests for cases where you currently don't do re-ordering (different access specifiers, partial initializers). Other than that, yes, this is fine to commit. Repository: rL LLVM https://reviews.llvm.org/D23279 _

Re: [PATCH] D23279: clang-reorder-fields

alexshap marked 2 inline comments as done. alexshap added a comment. Repository: rL LLVM https://reviews.llvm.org/D23279 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] D23279: clang-reorder-fields

alexshap added a comment. Okay, many thanks for the code review. May i commit this ? I will add more tests as a follow-up Repository: rL LLVM https://reviews.llvm.org/D23279 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llv

Re: [PATCH] D23279: clang-reorder-fields

djasper added inline comments. Comment at: clang-reorder-fields/ReorderFieldsAction.cpp:179 @@ +178,3 @@ +const ASTContext &Context, +std::map &Replacements) { + assert(InitListEx && "Init list expression is null"); alexshap wrote: > >Also, I don't unders

Re: [PATCH] D23279: clang-reorder-fields

djasper accepted this revision. djasper added a comment. This revision is now accepted and ready to land. Now that you can, you should add test cases for the different cases where you cannot reorder fields. Otherwise looks good. Repository: rL LLVM https://reviews.llvm.org/D23279 ___

Re: [PATCH] D23279: clang-reorder-fields

alexshap marked 4 inline comments as done. alexshap added a comment. Repository: rL LLVM https://reviews.llvm.org/D23279 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] D23279: clang-reorder-fields

alexshap added inline comments. Comment at: clang-reorder-fields/ReorderFieldsAction.cpp:179 @@ +178,3 @@ +const ASTContext &Context, +std::map &Replacements) { + assert(InitListEx && "Init list expression is null"); >Also, I don't understand what the pro

Re: [PATCH] D23279: clang-reorder-fields

alexshap updated this revision to Diff 69769. alexshap marked an inline comment as not done. alexshap added a comment. Check the return value of reorderFieldsInInitListExpr. Repository: rL LLVM https://reviews.llvm.org/D23279 Files: CMakeLists.txt clang-reorder-fields/CMakeLists.txt cl

Re: [PATCH] D23279: clang-reorder-fields

djasper added inline comments. Comment at: clang-reorder-fields/ReorderFieldsAction.cpp:178 @@ +177,3 @@ +const InitListExpr *InitListEx, ArrayRef NewFieldsOrder, +const ASTContext &Context, +std::map &Replacements) { alexshap wrote: > yeah, i am aware

Re: [PATCH] D23279: clang-reorder-fields

alexshap marked 5 inline comments as done. alexshap added a comment. Repository: rL LLVM https://reviews.llvm.org/D23279 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] D23279: clang-reorder-fields

alexshap added inline comments. Comment at: clang-reorder-fields/ReorderFieldsAction.cpp:83 @@ +82,3 @@ +static void +addReplacement(SourceRange Old, SourceRange New, const ASTContext &Context, + std::map &Replacements) { khm, it Replaces Old by New,

Re: [PATCH] D23279: clang-reorder-fields

alexshap added a reviewer: djasper. alexshap updated this revision to Diff 69748. alexshap added a comment. Address @djasper 's comments Repository: rL LLVM https://reviews.llvm.org/D23279 Files: CMakeLists.txt clang-reorder-fields/CMakeLists.txt clang-reorder-fields/ReorderFieldsActio

Re: [PATCH] D23279: clang-reorder-fields

djasper added a comment. Probably the last round .. Comment at: clang-reorder-fields/ReorderFieldsAction.cpp:202 @@ +201,3 @@ +for (const auto *C : RD->ctors()) { + if (C->isImplicit() || C->isDelegatingConstructor()) +continue; alexshap wrote:

Re: [PATCH] D23279: clang-reorder-fields

alexshap added a comment. F2347517: pingu Repository: rL LLVM https://reviews.llvm.org/D23279 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] D23279: clang-reorder-fields

alexshap added a comment. I've just rerun the tests - they are green. Repository: rL LLVM https://reviews.llvm.org/D23279 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] D23279: clang-reorder-fields

alexshap marked 10 inline comments as done. alexshap added a comment. I have marked all the inline comments as done, if i need to change/fix smth else - pls, reopen the corresponding comment or add a new one. **Current apporach** : correctly create all the necessary replacements supported by thi

Re: [PATCH] D23279: clang-reorder-fields

alexshap updated this revision to Diff 69445. alexshap added a comment. Address @djasper 's comments. 1. Try to flash out comments for helper functions / various interesting places in my code 2. More elaborate error-handling 3. Adjust the names of the variables: I -> i, E -> e 4. Minor cleanup &

Re: [PATCH] D23279: clang-reorder-fields

alexshap marked 4 inline comments as done. alexshap added a comment. Repository: rL LLVM https://reviews.llvm.org/D23279 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] D23279: clang-reorder-fields

alexshap marked 2 inline comments as done. Comment at: clang-reorder-fields/ReorderFieldsAction.cpp:202 @@ +201,3 @@ +for (const auto *C : RD->ctors()) { + if (C->isImplicit() || C->isDelegatingConstructor()) +continue; djasper wrote: > Why are yo

Re: [PATCH] D23279: clang-reorder-fields

alexshap added inline comments. Comment at: clang-reorder-fields/ReorderFieldsAction.cpp:165 @@ +164,3 @@ +return; + assert(InitListEx->getNumInits() == NewFieldsOrder.size() && + "Currently only full initialization is supported"); djasper wrote: > Sa

Re: [PATCH] D23279: clang-reorder-fields

djasper added inline comments. Comment at: clang-reorder-fields/ReorderFieldsAction.cpp:29 @@ +28,3 @@ +using namespace llvm; +using namespace clang; +using namespace clang::ast_matchers; Put everything here into the namespace clang and remove "using namespace cl

Re: [PATCH] D23279: clang-reorder-fields

alexshap added a comment. F2334952: pingpingpingping Repository: rL LLVM https://reviews.llvm.org/D23279 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cf

Re: [PATCH] D23279: clang-reorder-fields

alexshap added a comment. so looks like i have marked all the inline comments as done - please, let me know if i need to change smth else. Many thanks for the code review/comments/suggestions. Repository: rL LLVM https://reviews.llvm.org/D23279

Re: [PATCH] D23279: clang-reorder-fields

alexshap marked an inline comment as done. alexshap added a comment. Repository: rL LLVM https://reviews.llvm.org/D23279 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] D23279: clang-reorder-fields

alexshap updated this revision to Diff 69186. alexshap added a comment. Use a more elaborate approach for command line options. alexshap-mbp:srcs alexshap$ clang-reorder-fields -record-name Foo -fields-order s1,x,z,s2 main.cpp >/dev/null && echo ok ok alexshap-mbp:srcs alexshap$ clang-reorder-fi

Re: [PATCH] D23279: clang-reorder-fields

alexshap marked 17 inline comments as done. alexshap added a comment. Repository: rL LLVM https://reviews.llvm.org/D23279 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] D23279: clang-reorder-fields

alexshap updated this revision to Diff 69174. alexshap added a comment. Address @djasper 's comments 1. Add brief comments to the helper functions. 2. Get rid of the workaround. (https://reviews.llvm.org/D23828 has been merged in and i have rebased on the latest revision) 3. Made stylistic / n

Re: [PATCH] D23279: clang-reorder-fields

djasper added inline comments. Comment at: clang-reorder-fields/ReorderFieldsAction.cpp:137 @@ +136,3 @@ +std::end(NewWrittenInitializersOrder), ByFieldNewPosition); + assert(OldWrittenInitializersOrder.size() == + NewWrittenInitializersOrder.size());

Re: [PATCH] D23279: clang-reorder-fields

ioeric added inline comments. Comment at: clang-reorder-fields/ReorderFieldsAction.h:31 @@ +30,3 @@ + +public: + ReorderFieldsAction( alexshap wrote: > alexshap wrote: > > djasper wrote: > > > I don't why clang-rename does what it does at the moment. My main poin

Re: [PATCH] D23279: clang-reorder-fields

alexshap added a subscriber: ioeric. Comment at: clang-reorder-fields/ReorderFieldsAction.h:31 @@ +30,3 @@ + +public: + ReorderFieldsAction( alexshap wrote: > djasper wrote: > > I don't why clang-rename does what it does at the moment. My main point is > > that

Re: [PATCH] D23279: clang-reorder-fields

alexshap added inline comments. Comment at: clang-reorder-fields/ReorderFieldsAction.cpp:152 @@ +151,3 @@ + // SourceLocation::isValid is const. + if (!const_cast(InitListEx)->isExplicit() || + !InitListEx->getNumInits()) djasper wrote: > I think it is an o

Re: [PATCH] D23279: clang-reorder-fields

alexshap marked 2 inline comments as done. alexshap added a comment. Repository: rL LLVM https://reviews.llvm.org/D23279 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] D23279: clang-reorder-fields

alexshap added a comment. regarding writing the comments - yeah, i will add them - just have not updated the diff yet (writing comments doesn't come to me easily) Repository: rL LLVM https://reviews.llvm.org/D23279 ___ cfe-commits mailing list c

Re: [PATCH] D23279: clang-reorder-fields

alexshap added inline comments. Comment at: clang-reorder-fields/ReorderFieldsAction.h:31 @@ +30,3 @@ + +public: + ReorderFieldsAction( djasper wrote: > I don't why clang-rename does what it does at the moment. My main point is > that we have already implemented

Re: [PATCH] D23279: clang-reorder-fields

alexshap added inline comments. Comment at: clang-reorder-fields/tool/ClangReorderFields.cpp:44 @@ +43,3 @@ + bool parse(cl::Option &O, StringRef ArgName, const std::string &ArgValue, + SmallVector &Val) { +Val.clear(); djasper wrote: > Don't use

Re: [PATCH] D23279: clang-reorder-fields

djasper added a comment. I am serious about writing more comments. Comment at: clang-reorder-fields/ReorderFieldsAction.cpp:41 @@ +40,3 @@ + } + if (Results.size() != 1) { +errs() << "The name " << RecordName Make this "> 1" instead of "!= 1". Not really i

Re: [PATCH] D23279: clang-reorder-fields

alexshap updated this revision to Diff 69065. alexshap added a comment. minor cleanup Repository: rL LLVM https://reviews.llvm.org/D23279 Files: CMakeLists.txt clang-reorder-fields/CMakeLists.txt clang-reorder-fields/ReorderFieldsAction.cpp clang-reorder-fields/ReorderFieldsAction.h

Re: [PATCH] D23279: clang-reorder-fields

alexshap added inline comments. Comment at: clang-reorder-fields/ReorderFieldsAction.h:30 @@ +29,3 @@ + std::map &Replacements; + +public: the thing is that a few weeks ago there was a diff https://reviews.llvm.org/D21748?id=64016. That diff contains a lot of cha

Re: [PATCH] D23279: clang-reorder-fields

alexshap marked 2 inline comments as done. alexshap added a comment. Repository: rL LLVM https://reviews.llvm.org/D23279 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] D23279: clang-reorder-fields

alexshap updated this revision to Diff 69056. alexshap added a comment. Drop the callbacks (the worst-case complexity might be worse, but this approach requires less code). Repository: rL LLVM https://reviews.llvm.org/D23279 Files: CMakeLists.txt clang-reorder-fields/CMakeLists.txt cl

Re: [PATCH] D23279: clang-reorder-fields

djasper added a comment. Sorry, didn't have much time so far, will give this some more review later today. One thing those is that the code could do with a bit more comments. It would help reviewing (and future maintenance) of functions such as reorderFieldsInConstructor to have brief comment (

Re: [PATCH] D23279: clang-reorder-fields

bcraig added a comment. In https://reviews.llvm.org/D23279#523449, @alexshap wrote: > @djasper, @bcraig, @aaron.ballman - many thanks for the comments and > suggestions, > please, let me know if you are waiting for any other changes on my side (for > v0) or if there is anything else i can do t

Re: [PATCH] D23279: clang-reorder-fields

alexshap added a comment. @djasper, @bcraig, @aaron.ballman - many thanks for the comments and suggestions, please, let me know if you are waiting for any other changes on my side (for v0) or if there is anything else i can do to make reviewing easier. Repository: rL LLVM https://reviews.ll

Re: [PATCH] D23279: clang-reorder-fields

alexshap marked 4 inline comments as done. alexshap added a comment. https://reviews.llvm.org/D23279 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] D23279: clang-reorder-fields

alexshap removed rL LLVM as the repository for this revision. alexshap updated this revision to Diff 68955. alexshap added a comment. Get rid of sed in tests https://reviews.llvm.org/D23279 Files: CMakeLists.txt clang-reorder-fields/CMakeLists.txt clang-reorder-fields/ReorderFieldsAction.

Re: [PATCH] D23279: clang-reorder-fields

alexshap added a comment. > but by this moment i have not figured out yet how to make it happen. oh, i see, let me check - i will update this diff Repository: rL LLVM https://reviews.llvm.org/D23279 ___ cfe-commits mailing list cfe-commits@list

Re: [PATCH] D23279: clang-reorder-fields

alexshap added a comment. @aaron.ballman thanks, yeah, i understand ur concerns that now we have zero tests that can run on windows. To be honest i would prefer not to depend on sed, cat etc at all and have most of the tests (i would prefer all of them) running on windows - but by this moment i

Re: [PATCH] D23279: clang-reorder-fields

alexshap updated this revision to Diff 68953. alexshap added a comment. Drop support of -field-pos for now Repository: rL LLVM https://reviews.llvm.org/D23279 Files: CMakeLists.txt clang-reorder-fields/CMakeLists.txt clang-reorder-fields/ReorderFieldsAction.cpp clang-reorder-fields/R

Re: [PATCH] D23279: clang-reorder-fields

djasper added a comment. I think quite a bit of the complexity in this patch stems from the two different ways to format the input (defining the field order or alternatively defining the new index of specific fields). Do we really need both? If not, I'd remove one of them for now (likely the in

Re: [PATCH] D23279: clang-reorder-fields

aaron.ballman added a comment. In https://reviews.llvm.org/D23279#522779, @alexshap wrote: > @aaron.ballman > Thanks, yeah, there is an issue. Also there are inline-comments by @bcraig > and @compnerd about the tests. > I used clang-rename, include-fixer, clang-tidy as examples. Correct, but

Re: [PATCH] D23279: clang-reorder-fields

alexshap added a comment. @aaron.ballman Thanks, yeah, there is an issue. Also there are inline-comments by @bcraig and @compnerd about the tests. I used clang-rename, include-fixer, clang-tidy as examples. alexshap-mbp:extra alexshap$ grep -r -n "sed -e " ./* | head -n 5 ./test/clang-tidy/mode

Re: [PATCH] D23279: clang-reorder-fields

aaron.ballman added a comment. None of your tests will run on Windows, which makes me worried if that's intended to be a supported target. https://reviews.llvm.org/D23279 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org

Re: [PATCH] D23279: clang-reorder-fields

alexshap removed rL LLVM as the repository for this revision. alexshap changed the visibility of this Differential Revision from "All Users" to "Public (No Login Required)". alexshap updated this revision to Diff 68943. alexshap added a comment. Fix typo https://reviews.llvm.org/D23279 Files:

Re: [PATCH] D23279: clang-reorder-fields

alexshap updated this revision to Diff 68939. alexshap added a comment. 1. Get rid of the visitor. 2. Get rid of the dependency on libc++ in tests (ran and checked the tests under the minimal checkout: llvm + clang + extra tools) Repository: rL LLVM https://reviews.llvm.org/D23279 Files:

Re: [PATCH] D23279: clang-reorder-fields

alexshap added a comment. @djasper - regarding the visitor - thank you, i understand ur concerns - i will add a version which uses a matcher (will update this diff). Repository: rL LLVM https://reviews.llvm.org/D23279 ___ cfe-commits mailing lis

Re: [PATCH] D23279: clang-reorder-fields

alexshap added inline comments. Comment at: test/clang-reorder-fields/CStructAmbiguousName.cpp:6 @@ +5,3 @@ +struct Foo { + int x;// CHECK: int x; + double y; // CHECK: double y; djasper wrote: > Have you thought about how to handle comments that surround th

Re: [PATCH] D23279: clang-reorder-fields

djasper added a comment. Ben: I am happy to have this as a separate tool until then. But if we go that route, let's add very explicit comments about this so that people don't start depending on it and we can actually remove it as easily as possible. Alex: I don't think there will be a significa

Re: [PATCH] D23279: clang-reorder-fields

bcraig added a comment. In https://reviews.llvm.org/D23279#522504, @djasper wrote: > In the meantime, I don't know whether we should check this in as a > standalone-tool and then merge into clang-refactor once ready. Ben, what do > you think? Well, I don't know how much you should count my op

Re: [PATCH] D23279: clang-reorder-fields

alexshap added a comment. > My only high-level comment so far (will try to review in more detail later) > is: I find it strange to mix ASTMatchers and a >RecursiveASTVisitor in the > same tool. It seem that if you are using ASTMatchers internally for other > things, you should also >use ASTMatc

Re: [PATCH] D23279: clang-reorder-fields

djasper added a subscriber: djasper. djasper added a comment. Sorry for the long silence. Manuel is currently out on vacation. I am not entirely sure how we want to make progress at this point. There seems to be consensus that in the mid-term, this should go into the clang-refactor tool. Howeve

Re: [PATCH] D23279: clang-reorder-fields

alexshap added a comment. Thanks for the comments. @bcraig, multi-TU support is in - in the sense that it's in at the same level, as it's in for clang-rename and the other tools (take a look my comments above, please). Also: @omtcyf0: > Well, multi-TU support is needed for many tools and it is

Re: [PATCH] D23279: clang-reorder-fields

compnerd added inline comments. Comment at: test/clang-reorder-fields/ClassMixedInitialization.cpp:1-3 @@ +1,4 @@ +// RUN: cat %s > %t.cpp +// RUN: clang-reorder-fields -record-name Foo -fields-order e,x,pi,v,s %t.cpp -i -- -std=c++11 +// RUN: sed 's,//.*,,' %t.cpp | FileCheck %s

Re: [PATCH] D23279: clang-reorder-fields

On Mon, Aug 22, 2016 at 10:34 AM, Ben Craig wrote: > bcraig added a comment. > > I will note that I'm only really looking at the tests, mostly from the > perspective of a potential user. > > Thanks for getting C++ initializer lists in. > > The tests in general look fine. The omission of cross vi

Re: [PATCH] D23279: clang-reorder-fields

bcraig added a comment. I will note that I'm only really looking at the tests, mostly from the perspective of a potential user. Thanks for getting C++ initializer lists in. The tests in general look fine. The omission of cross visibility reordering is fine as an initial step. Once multi-TU s

Re: [PATCH] D23279: clang-reorder-fields

alexshap added a comment. ping Repository: rL LLVM https://reviews.llvm.org/D23279 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] D23279: clang-reorder-fields

alexshap removed rL LLVM as the repository for this revision. alexshap updated this revision to Diff 68738. alexshap added a comment. 1. Handle C++ ctors. 2. Add tests. 3. I have a diff for proper moving the attached comments based on http://llvm.org/devmtg/2012-11/Gribenko_CommentParsing.pdf . I

Re: [PATCH] D23279: clang-reorder-fields

alexshap added a comment. > Well, multi-TU support is needed for many tools and it is far beyond one tool > implementation details if we want it to be nice. I want to >have multi-TU > support in the clang-refactor, but it is still in designing stage at the > moment. At first, one translation un

Re: [PATCH] D23279: clang-reorder-fields

alexshap added a comment. 0. the tool relies on the compilation database from day one (like the other clang tools using tooling::RefactoringTool) 1. regarding TUs:. A. just in case how it works: it uses the full name of record to find the definition, then it changes the definition, then it fin

Re: [PATCH] D23279: clang-reorder-fields

omtcyf0 added a subscriber: omtcyf0. omtcyf0 added a comment. In https://reviews.llvm.org/D23279#511439, @bcraig wrote: > I will agree that compilation databases are a (mostly) necessary, but not > sufficient part of the solution. The refactoring tool will need to be > careful about changing t

Re: [PATCH] D23279: clang-reorder-fields

bcraig added a comment. In https://reviews.llvm.org/D23279#511266, @omtcyfz wrote: > In https://reviews.llvm.org/D23279#511202, @bcraig wrote: > > > In https://reviews.llvm.org/D23279#511187, @omtcyfz wrote: > > > > > In https://reviews.llvm.org/D23279#510234, @alexshap wrote: > > > > > > > > I d

Re: [PATCH] D23279: clang-reorder-fields

omtcyfz added a comment. In https://reviews.llvm.org/D23279#511202, @bcraig wrote: > In https://reviews.llvm.org/D23279#511187, @omtcyfz wrote: > > > In https://reviews.llvm.org/D23279#510234, @alexshap wrote: > > > > > > I do think that an automated tool will do a better job of changing > > > >

Re: [PATCH] D23279: clang-reorder-fields

bcraig added a comment. In https://reviews.llvm.org/D23279#511187, @omtcyfz wrote: > In https://reviews.llvm.org/D23279#510234, @alexshap wrote: > > > > I do think that an automated tool will do a better job of changing field > > > orderings in a non-breaking way than most people would, mostly d

Re: [PATCH] D23279: clang-reorder-fields

omtcyfz added a comment. In https://reviews.llvm.org/D23279#510234, @alexshap wrote: > > I do think that an automated tool will do a better job of changing field > > orderings in a non-breaking way than most people would, mostly due to > > initializer lists. > > > yeah, that was the original mo

Re: [PATCH] D23279: clang-reorder-fields

alexshap added a comment. > I do think that an automated tool will do a better job of changing field > orderings in a non-breaking way than most people would, mostly due to > initializer lists. yeah, that was the original motivation Repository: rL LLVM https://reviews.llvm.org/D23279 _

Re: [PATCH] D23279: clang-reorder-fields

bcraig added a comment. In https://reviews.llvm.org/D23279#510178, @omtcyfz wrote: > In https://reviews.llvm.org/D23279#510167, @alexshap wrote: > > > my point is the following: this tool helps perform some operation (changing > > the order of fields), > > if smb decides to do it manually - th

Re: [PATCH] D23279: clang-reorder-fields

omtcyfz added a comment. In https://reviews.llvm.org/D23279#510167, @alexshap wrote: > my point is the following: this tool helps perform some operation (changing > the order of fields), > if smb decides to do it manually - the result will have exactly the same > issue. While creating tools

Re: [PATCH] D23279: clang-reorder-fields

alexshap added a comment. my point is the following: this tool helps perform some operation (changing the order of fields), if smb decides to do it manually - the result will have exactly the same issue. Repository: rL LLVM https://reviews.llvm.org/D23279

Re: [PATCH] D23279: clang-reorder-fields

omtcyfz added a comment. In https://reviews.llvm.org/D23279#510157, @alexshap wrote: > The difference is that if we reorder fields the new struct is not > binary-compatible with the old one. Exactly. If both libraries are in your project, it becomes broken. Repository: rL LLVM https://rev

Re: [PATCH] D23279: clang-reorder-fields

alexshap added a comment. The difference is that if we reorder fields the new struct is not binary-compatible with the old one. Repository: rL LLVM https://reviews.llvm.org/D23279 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://li

Re: [PATCH] D23279: clang-reorder-fields

omtcyfz added a comment. In https://reviews.llvm.org/D23279#510147, @alexshap wrote: > Suppose you have a struct Foo defined in LibA and you have used clang-rename > to rename Foo into Bar (or just a field of Foo say - you have renamed Foo::x > into Foo::y); LibA is linked to LibB and you have

Re: [PATCH] D23279: clang-reorder-fields

alexshap added a comment. Suppose you have a struct Foo defined in LibA and you have used clang-rename to rename Foo into Bar (or just a field of Foo say - you have renamed Foo::x into Foo::y); LibA is linked to LibB and you have Foo foo = { &x, 17, 1.29, 0 }; in LibB. Therefore LibA knows noth

Re: [PATCH] D23279: clang-reorder-fields

omtcyfz added a comment. In https://reviews.llvm.org/D23279#510129, @alexshap wrote: > Regarding TUs - i am aware of that - would be useful if you could provide a > test which doesn't work - that would help us avoid miscommunication. Thanks. Suppose you have a `struct Foo` defined in `LibA` an

Re: [PATCH] D23279: clang-reorder-fields

alexshap added a comment. Regarding TUs - i am aware of that - would be useful if you could provide a test which doesn't work - that would help us avoid miscommunication. Thanks. Repository: rL LLVM https://reviews.llvm.org/D23279 ___ cfe-commit

Re: [PATCH] D23279: clang-reorder-fields

omtcyfz added a comment. In https://reviews.llvm.org/D23279#510061, @alexshap wrote: > First of all, many thanks for the comments & proposal of clang-refactor. > > 1. Regarding high-level project organization: clang-refactor - that can be a > good place for various refactoring techniques like th

Re: [PATCH] D23279: clang-reorder-fields

alexshap added a comment. > The excess padding checker doesn't provide a fix-it. yup, although i think it would be useful to add one of the optimal layouts to the report (i have a diff for that (still in the oven), i have not sent it for code review yet). But as i mentioned above - it seems to

Re: [PATCH] D23279: clang-reorder-fields

bcraig added a comment. It is very common for developers to put comments near a field. If this tool doesn't move the "anchored" comments, then its utility will be severely limited. Does this tool also reorder C++ member initializer lists? If it doesn't, then running this tool will introduce w

Re: [PATCH] D23279: clang-reorder-fields

bcraig added a subscriber: bcraig. bcraig added a comment. In https://reviews.llvm.org/D23279#509017, @Eugene.Zelenko wrote: > Do we really need standalone tool for this purpose? If I'm not mistaken, > Static Analyzer already has clang-analyzer-optin.performance.Padding check, > which is also a

Re: [PATCH] D23279: clang-reorder-fields

alexshap added a comment. First of all, many thanks for the comments & proposal of clang-refactor. 1. Regarding high-level project organization: clang-refactor - that can be a good place for various refactoring techniques like the existing clang-rename, my simple tool, etc - essentially what Kir

Re: [PATCH] D23279: clang-reorder-fields

On Tue, Aug 9, 2016 at 12:14 PM, Kirill Bobyrev wrote: > omtcyfz added a comment. > > In https://reviews.llvm.org/D23279#510011, @aaron.ballman wrote: > >> In https://reviews.llvm.org/D23279#510002, @omtcyfz wrote: >> >> > In https://reviews.llvm.org/D23279#509896, @compnerd wrote: >> > >> > > If

Re: [PATCH] D23279: clang-reorder-fields

omtcyfz added a comment. In https://reviews.llvm.org/D23279#510011, @aaron.ballman wrote: > In https://reviews.llvm.org/D23279#510002, @omtcyfz wrote: > > > In https://reviews.llvm.org/D23279#509896, @compnerd wrote: > > > > > If the argument really is that we want to minimize the tools then Id

Re: [PATCH] D23279: clang-reorder-fields

aaron.ballman added a comment. In https://reviews.llvm.org/D23279#510002, @omtcyfz wrote: > In https://reviews.llvm.org/D23279#509896, @compnerd wrote: > > > If the argument really is that we want to minimize the tools then Id argue > > that `clang-rename` also belongs in `clang-tidy` as it woul

Re: [PATCH] D23279: clang-reorder-fields

omtcyfz added a comment. In https://reviews.llvm.org/D23279#509896, @compnerd wrote: > If the argument really is that we want to minimize the tools then Id argue > that `clang-rename` also belongs in `clang-tidy` as it would be used to > rename fields to match the naming convention (tidying up

  1   2   >