berenm added a comment.
In https://reviews.llvm.org/D21279#550565, @djasper wrote:
> Are we talking completely past each other? I specifically think we should
> *NOT* combine NestingLevel and IndentLevel into one value. Not in
> ScopeLevel() and not anywhere else.
Ok, I probably
berenm added a comment.
Hello,
I had a little bit of look into the NestingLevel field. I understand that it
only indicates the nesting level of the token inside the current unwrapped
line, which could very well be the same as the nesting level of another token
in the previous or next
berenm added a comment.
In http://reviews.llvm.org/D21279#462578, @bmharper wrote:
> 2. friend functions: I don't really understand why the current behavior is
> what it is, but I think it's reasonable to argue that it actually improves
> readability by drawing attention to the fact these are
berenm added a comment.
Thanks! The operators are now correctly handled.
Another thing I've found is that constructors aren't aligned either with other
declarations or either together. Do you think it would be possible to treat
them as functions as well?
Friend functions also aren't aligned
berenm added a comment.
It looks like it doesn't like the `operator[]` either:
struct test {
long long int foo();
int operator[](int a);
double bar();
long long int foo();
int operator()(int a);
double bar();
};
becomes:
struct test {
long long int foo();
berenm added a comment.
There's still cases where the nesting level is still not correctly handled:
when using unbraced conditionals / loops. For example (sorry, silly example):
for (auto index = 0, e = 1000; index < e; ++index)
int v = 0;
long double value = 1;
is aligned to
for
berenm added a comment.
I think it's better to update the diff with full context (git diff -U99)
rather than attach the file, in order to have it integrated into phabricator
diff view. Or use the arcanist command-line tool, which should do it for you.
Except from that, from a quick reading
berenm marked 8 inline comments as done.
berenm added a comment.
For information, I just tested the current diff under Visual Studio 2010 SP1
and everything looks fine as well.
http://reviews.llvm.org/D12407
___
cfe-commits mailing list
berenm added a comment.
Actually, the main issue that I see is the difficulty to build the plugin
itself. The project does not build out-of-the-box on VS2013, and I had to
change the assembly references by hand.
I have a patch to get the required assemblies in a more portable way using
nuget
berenm updated this revision to Diff 42718.
berenm added a comment.
- Patch rebased on trunk
- Small cleanups in the RunningDocumentTable usage
- Tested and appears work on VS >= 2012
http://reviews.llvm.org/D12407
Files:
tools/clang-format-vs/ClangFormat/ClangFormat.csproj
berenm added inline comments.
Comment at: tools/clang-format-vs/ClangFormat/ClangFormatPackage.cs:195
@@ -179,1 +194,3 @@
+runningDocumentTable = new RunningDocumentTable(this);
+adviseCookie = runningDocumentTable.Advise(this);
In this
berenm added a comment.
I don't have commit access, if you don't mind landing it for me :)
http://reviews.llvm.org/D14325
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
berenm updated this revision to Diff 41296.
berenm added a comment.
[clang-format] Code cleanup and variable naming in
WhitespaceManager::AlignTokens
http://reviews.llvm.org/D14325
Files:
lib/Format/WhitespaceManager.cpp
lib/Format/WhitespaceManager.h
unittests/Format/FormatTest.cpp
berenm updated this revision to Diff 41299.
berenm marked an inline comment as done.
berenm added a comment.
Fix continue statements.
http://reviews.llvm.org/D14325
Files:
lib/Format/WhitespaceManager.cpp
lib/Format/WhitespaceManager.h
unittests/Format/FormatTest.cpp
Index:
berenm marked 2 inline comments as done.
Comment at: lib/Format/WhitespaceManager.cpp:197
@@ +196,3 @@
+ // Keep track of the nesting level of matching tokens, i.e. the number of
+ // surrounding (), [], or {}. We will only align a sequence of matching
+ // token that share
berenm added inline comments.
Comment at: lib/Format/WhitespaceManager.cpp:242-243
@@ -320,34 +241,4 @@
}
-if (Changes[i].Kind == tok::r_brace) {
- if (!FoundLeftBraceOnLine)
-AlignSequence();
- FoundLeftBraceOnLine = false;
-} else if
berenm added a comment.
Updated the diff with a fix for the issue reported in
http://lists.llvm.org/pipermail/cfe-dev/2015-November/046057.html
http://reviews.llvm.org/D14325
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
berenm updated this revision to Diff 41271.
berenm added a comment.
[clang-format] alignConsecutiveXXX: replace the buggy paren / braces counter
with a better scope tracker.
http://reviews.llvm.org/D14325
Files:
lib/Format/WhitespaceManager.cpp
lib/Format/WhitespaceManager.h
berenm marked an inline comment as done.
berenm added a comment.
Ping?
http://reviews.llvm.org/D14325
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
berenm added inline comments.
Comment at: tools/clang-format-vs/ClangFormat/ClangFormatPackage.cs:86
@@ -69,1 +85,3 @@
+IComponentModel componentModel =
GetService(typeof(SComponentModel)) as IComponentModel;
+editorAdaptersFactoryService =
berenm marked an inline comment as done.
Comment at: lib/Format/WhitespaceManager.cpp:150-151
@@ -149,109 +149,4 @@
}
-// Walk through all of the changes and find sequences of "=" to align. To do
-// so, keep track of the lines and whether or not an "=" was found on align. If
berenm updated this revision to Diff 39232.
berenm added a comment.
[clang-format] Alignment code factorization.
http://reviews.llvm.org/D14325
Files:
lib/Format/WhitespaceManager.cpp
unittests/Format/FormatTest.cpp
Index: unittests/Format/FormatTest.cpp
berenm updated this revision to Diff 39233.
berenm added a comment.
[clang-format] Remove deleted methods declaration.
http://reviews.llvm.org/D14325
Files:
lib/Format/WhitespaceManager.cpp
lib/Format/WhitespaceManager.h
unittests/Format/FormatTest.cpp
Index:
berenm updated this revision to Diff 39201.
berenm added a comment.
[clang-format] Count the number of braces and parens on the line instead of
remembering only one.
http://reviews.llvm.org/D14325
Files:
lib/Format/WhitespaceManager.cpp
unittests/Format/FormatTest.cpp
Index:
berenm added a comment.
I've also added a fix for the other issue reported on the same bug. I could
split the two reviews if necessary.
http://reviews.llvm.org/D14325
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
berenm created this revision.
berenm added a reviewer: djasper.
berenm added a subscriber: cfe-commits.
Herald added a subscriber: klimek.
This fixes bug #25329, as well as misalignments like the following:
int a, b = 2;
int c= 3;
http://reviews.llvm.org/D14325
Files:
berenm added inline comments.
Comment at: unittests/Format/FormatTest.cpp:8657
@@ +8656,3 @@
+ "};\n"
+ "int i = 0;\n"
+ "auto v = type{\n"
djasper wrote:
> So, it is kind of interesting that we (would) align with a
berenm created this revision.
berenm added a reviewer: djasper.
berenm added a subscriber: cfe-commits.
Herald added a subscriber: klimek.
This was done correctly when aligning the declarations, but not when aligning
assignments.
FIXME: The code between assignments and declarations alignment is
berenm added a comment.
Alright, thank you for your time!
Repository:
rL LLVM
http://reviews.llvm.org/D13081
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
berenm marked 7 inline comments as done.
Comment at: unittests/Format/FormatTest.cpp:8699
@@ +8698,3 @@
+ "int one = 1;\n"
+ "\n"
+ "unsigned oneTwoThree = 123;",
I think I actually managed to do
berenm marked 2 inline comments as done.
berenm added a comment.
http://reviews.llvm.org/D12362
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
berenm added a comment.
Thanks, I don't have commit access.
http://reviews.llvm.org/D12362
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
berenm added inline comments.
Comment at: clang-tidy/readability/IdentifierNamingCheck.cpp:545
@@ +544,3 @@
+ if (const auto *Loc = Result.Nodes.getNodeAs("typeLoc")) {
+if (const auto = Loc->getAs()) {
+ addUsage(NamingCheckFailures, Ref.getDecl(),
berenm updated this revision to Diff 35946.
berenm added a comment.
Update the diff with the ProvideAutoLoad attribute.
Thanks @MyDeveloperDay for the review and tips!
http://reviews.llvm.org/D12407
Files:
tools/clang-format-vs/ClangFormat/ClangFormat.csproj
berenm updated this revision to Diff 35972.
berenm added a comment.
Address the latest comments, as well as useless code removal.
- Only the start of the token range is stored, so there is no need to care
about the end of the range. Remove the code that was trying to match the end of
the range
berenm updated this revision to Diff 35974.
berenm added a comment.
Fix arcanist messup...
http://reviews.llvm.org/D13081
Files:
clang-tidy/readability/IdentifierNamingCheck.cpp
test/clang-tidy/Inputs/readability-identifier-naming/system/system-header.h
berenm added inline comments.
Comment at: unittests/Format/FormatTest.cpp:8699
@@ +8698,3 @@
+ Alignment));
+ Alignment.AlignConsecutiveAssignments = true;
+ verifyFormat("float something = 2000;\n"
djasper wrote:
> Can you add a case
berenm marked 3 inline comments as done.
Comment at: clang-tidy/readability/IdentifierNamingCheck.cpp:141
@@ +140,3 @@
+ Finder->addMatcher(usingDecl().bind("using"), this);
+ Finder->addMatcher(declRefExpr().bind("declRef"), this);
+
berenm marked an inline comment as done.
berenm added a comment.
I think the latest version also addresses http://reviews.llvm.org/D13079 in a
better way. The check will now let clang-tidy decide whether warnings and fixes
are displayed, whether it is in or from system/user header files or in
berenm marked an inline comment as done.
berenm added a comment.
http://reviews.llvm.org/D13081
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
berenm added a comment.
Well, the format on save setting is disabled by default, do you mean you had to
enable it twice? Or you had it enabled without the C++ development tools, and
after the installation you had to disable and enable it again?
http://reviews.llvm.org/D12407
berenm updated this revision to Diff 35669.
berenm added a comment.
Reformatting with clang-format
http://reviews.llvm.org/D13079
Files:
clang-tidy/readability/IdentifierNamingCheck.cpp
clang-tidy/readability/IdentifierNamingCheck.h
test/clang-tidy/readability-identifier-naming.cpp
berenm added a comment.
This will also disable all warnings for declaration / usages outside of the
main file.
It might be better to disable the warnings and fixes whenever a macro is
involved (in the declaration or any usage), but at least keep the warning
across files, even if we don't
berenm marked 2 inline comments as done.
berenm added a comment.
http://reviews.llvm.org/D13079
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
berenm updated this revision to Diff 35672.
berenm added a comment.
- Do not check for identifier names from system headers
- Check for SourceLocation validity before modifying them
http://reviews.llvm.org/D13081
Files:
clang-tidy/readability/IdentifierNamingCheck.cpp
berenm created this revision.
berenm added a reviewer: alexfh.
berenm added a subscriber: cfe-commits.
This fixes https://llvm.org/bugs/show_bug.cgi?id=24835.
The patch might be slightly different after http://reviews.llvm.org/D13079 and
http://reviews.llvm.org/D13081, but the idea is the same.
berenm updated this revision to Diff 35468.
berenm added a comment.
Even better with the corresponding unit test fix.
http://reviews.llvm.org/D13090
Files:
clang-tidy/readability/IdentifierNamingCheck.cpp
test/clang-tidy/readability-identifier-naming.cpp
Index:
berenm changed the visibility of this Differential Revision from "berenm (Beren
Minor)" to "Public (No Login Required)".
berenm updated this revision to Diff 35465.
berenm added a comment.
Remove remaining commented code.
http://reviews.llvm.org/D13081
Files:
berenm updated this revision to Diff 35466.
berenm added a comment.
Fix incorrect line removal in unit test.
http://reviews.llvm.org/D13081
Files:
clang-tidy/readability/IdentifierNamingCheck.cpp
test/clang-tidy/readability-identifier-naming.cpp
Index:
berenm added inline comments.
Comment at: clang-tidy/readability/IdentifierNamingCheck.cpp:508
@@ +507,3 @@
+ auto = Failures[Decl];
+ for (const auto : Failure.Usages) {
+if (R == Range)
Hopefully the number of ranges in Usages will stay low. If not,
berenm added a comment.
Ping? Just to be sure it's not going to fall to oblivion. :)
http://reviews.llvm.org/D12407
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
berenm added a comment.
Ping?
The unit tests should work fine, now that http://reviews.llvm.org/D12369 has
been merged.
http://reviews.llvm.org/D12362
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
berenm added a comment.
I don't have credentials to commit it, anybody could do it for me if the diff
is ok?
Thanks!
http://reviews.llvm.org/D12369
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
berenm updated this revision to Diff 33783.
berenm added a comment.
Here it is, with the typos in comments corrected, and rebased over the latest
trunk.
Thanks for the review!
http://reviews.llvm.org/D12369
Files:
lib/Format/WhitespaceManager.cpp
unittests/Format/FormatTest.cpp
Index:
berenm marked 5 inline comments as done.
berenm added a comment.
http://reviews.llvm.org/D12369
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
berenm added a comment.
Sure, no worries.
http://reviews.llvm.org/D12407
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
berenm created this revision.
berenm added a reviewer: djasper.
berenm added a subscriber: cfe-commits.
By default, clang-format VS plugin only reformats the selected code.
To reformat the whole document, the user has to select everything before
calling the reformat shortcut.
Alright, my bad. It does indeed.
I was trying to add a Reformat all on save feature in the plugin, and
after struggling with VSSDK I thought this would be an easy first step.
--
Beren Minor
On Thu, Aug 27, 2015 at 3:36 PM, Aaron Ballman aa...@aaronballman.com
wrote:
On Thu, Aug 27, 2015 at
berenm updated this revision to Diff 33212.
berenm added a comment.
Added some test cases in combined usage with AlignConsecutiveAssignments.
The tests should pass with http://reviews.llvm.org/D12369 applied.
http://reviews.llvm.org/D12362
Files:
include/clang/Format/Format.h
berenm added a comment.
I'm not entirely sure of what was going on, so I rewrote the code a bit, and
hopefully, more clearly.
Comment at: lib/Format/WhitespaceManager.cpp:218
@@ -217,3 +229,1 @@
- bool AlignedAssignment = false;
- int PreviousShift = 0;
for (unsigned i =
berenm added a comment.
Actually, I think there is a bug in the assignment alignment code. Even without
this patch, this code doesn't align properly:
int oneTwoThree = 123;
int oneTwo = 12;
method();
I don't think I completely understand the assignment alignment code, and that's
berenm marked 2 inline comments as done.
Comment at: clang-tidy/readability/IdentifierNamingCheck.cpp:166
@@ +165,3 @@
+ static llvm::Regex Splitter(
+ (([a-z0-9A-Z]*)(_+)|([A-Z]?[a-z0-9]+)([A-Z]|$)|([A-Z]+)([A-Z]|$)));
+
alexfh wrote:
Why do you need the
berenm added a comment.
In http://reviews.llvm.org/D10933#225671, @alexfh wrote:
Looks good with a couple of comments. Tell me if you need me to submit the
patch for you, once you address the comments.
If you tell me how I should proceed, I can maybe do it myself (do I just have
to send
berenm updated this revision to Diff 32277.
berenm added a comment.
Here is an updated version with the latest comments fixed.
http://reviews.llvm.org/D10933
Files:
clang-tidy/readability/CMakeLists.txt
clang-tidy/readability/IdentifierNamingCheck.cpp
berenm marked 6 inline comments as done.
berenm added a comment.
http://reviews.llvm.org/D10933
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
berenm updated this revision to Diff 32294.
berenm added a comment.
Indeed, I probably don't have rights on Clang/LLVM repositories.
I have updated the patch with stricter tests - and found a missing break
statement (`clang-tidy/readability/IdentifierNamingCheck.cpp:201`).
I also realized that
66 matches
Mail list logo
