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 unwrapped
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 misunderstood
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
http://lists.llvm.org/cgi-bin/mailman/listi
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
http://lists.llvm.org/cgi-bin/mailman/li
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:
clang-tidy/readability/Identifie
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: test/clang-tidy/readability-identifie
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: test/clang-tidy/readability-
berenm added inline comments.
Comment at: clang-tidy/readability/IdentifierNamingCheck.cpp:508
@@ +507,3 @@
+ auto &Failure = Failures[Decl];
+ for (const auto &R : Failure.Usages) {
+if (R == Range)
Hopefully the number of ranges in Usages will stay low. If
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.
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 35651.
berenm added a comment.
Changed Usages to RawUsageLocs, as DenseMap storing the raw encoding of
SourceLocation instead of a vector or SourceRange.
http://reviews.llvm.org/D13079
Files:
clang-tidy/readability/IdentifierNamingCheck.cpp
clang-tidy/r
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 35652.
berenm added a comment.
Update the diff with more context, thanks to arcanist.
http://reviews.llvm.org/D13090
Files:
clang-tidy/readability/IdentifierNamingCheck.cpp
test/clang-tidy/readability-identifier-naming.cpp
Index: test/clang-tidy/readabi
berenm updated this revision to Diff 35655.
berenm added a comment.
Update the diff with more context.
http://reviews.llvm.org/D13081
Files:
clang-tidy/readability/IdentifierNamingCheck.cpp
test/clang-tidy/readability-identifier-naming.cpp
Index: test/clang-tidy/readability-identifier-nami
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
Index
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 offer
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
test/clang-tidy/readab
berenm updated this revision to Diff 35818.
berenm added a comment.
Added comments and reformated NamingCheckIdentifer.h with clang-format.
http://reviews.llvm.org/D13079
Files:
clang-tidy/readability/IdentifierNamingCheck.cpp
clang-tidy/readability/IdentifierNamingCheck.h
test/clang-tidy
berenm marked 3 inline comments as done.
berenm added a comment.
In http://reviews.llvm.org/D13079#253512, @alexfh wrote:
> Please tell me, if you need me to commit the patch for you after you address
> the comments.
Yes please, I cannot commit it myself.
http://reviews.llvm.org/D13079
__
berenm updated this revision to Diff 35819.
berenm added a comment.
Reworked the patches a little bit, addressing the comments and adding
better handling of checks in header files with corresponding unit test cases.
http://reviews.llvm.org/D13081
Files:
clang-tidy/readability/IdentifierNaming
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);
+ Finder->addMatcher(cxxConstructorDecl().bin
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 ma
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 abandoned this revision.
berenm added a comment.
Actually I believe it's better fixed by http://reviews.llvm.org/D13081.
http://reviews.llvm.org/D13090
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mail
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
tools/clang-format-vs/ClangForm
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 &Ref = Loc->getAs()) {
+ addUsage(NamingCheckFailures, Ref.getDecl(), Loc->getSourceRa
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
test/clang-tidy/Inputs/readability-
berenm marked 3 inline comments 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 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 (unl
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 updated this revision to Diff 36207.
berenm added a comment.
Fix arcanist insisting on creating the diff against old revision...
http://reviews.llvm.org/D12362
Files:
include/clang/Format/Format.h
lib/Format/Format.cpp
lib/Format/WhitespaceManager.cpp
lib/Format/WhitespaceManager
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 something
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 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 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 lambd
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 th
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
clang-tidy/readability/IdentifierNami
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 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 t
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
berenm updated this revision to Diff 32304.
berenm added a comment.
Alright!
Here is the latest revision then, with FIXMEs comments mentioning this missing
feature. I will work on implementing it and come back to you when i've got
something working.
Thanks for your time and your advices!
htt
berenm created this revision.
berenm added a reviewer: djasper.
berenm added a subscriber: cfe-commits.
Herald added a subscriber: klimek.
This allows clang-format to align identifiers in consecutive declarations.
This is, arguably, a feature useful for increasing the readability of the code,
in
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
why
berenm created this revision.
berenm added a reviewer: djasper.
berenm added a subscriber: cfe-commits.
Herald added a subscriber: klimek.
This simple test case (added to the unit tests) was failing to align,
apparently due to the ``method();`` on the last line:
```
int oneTwoThree = 123;
int on
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 = S
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
lib/Format/Fo
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.
http://reviews.ll
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
wrote:
> On Thu, Aug 27, 2015 at 9:34 AM, Daniel Jasper
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 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: un
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 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:
lib/Format/W
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: unittests/For
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
http://lists.llvm.org/cgi
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: unittests/Format/FormatTest.
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 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 =
componentMod
berenm updated this revision to Diff 31447.
berenm added a comment.
Here is an updated version with some style fixes, and function splits.
The styles are still selected the same way as before (no split between finding
the best type style and falling back to available style), but there is only th
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 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 (au
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.
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 wi
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 f
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 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
unittests/Fo
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
http://lists.llvm
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
Ind
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 the
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: unittests/For
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 (Changes[i].
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 added a comment.
I don't have any compilation error with GCC 5.2.1. I tried again with Clang 3.8
and indeed there is this error:
tools/clang/lib/Format/WhitespaceManager.cpp:155:51: error: 'Change' is a
private member of 'clang::format::WhitespaceManager'
SmallVect
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
tools/clang-form
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 b
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
cfe-commits
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 p
80 matches
Mail list logo
