takuto.ikuta accepted this revision.
takuto.ikuta added a comment.
This revision is now accepted and ready to land.
Seems good document!
https://reviews.llvm.org/D54319
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cg
stephanemoore updated this revision to Diff 173544.
stephanemoore marked 9 inline comments as done.
stephanemoore added a comment.
Updated with the following changes:
• Elided conditional braces to comply with LLVM style.
• Converted conditional loop break to loop condition in generateFixItHint.
•
hwright updated this revision to Diff 173543.
hwright marked 16 inline comments as done.
hwright added a comment.
Addressed reviewer feedback.
https://reviews.llvm.org/D54246
Files:
clang-tidy/abseil/AbseilTidyModule.cpp
clang-tidy/abseil/CMakeLists.txt
clang-tidy/abseil/DurationFactorySc
hwright added inline comments.
Comment at: clang-tidy/abseil/DurationFactoryScaleCheck.cpp:74
+ case DurationScale::Minutes:
+if (Multiplier == 60.0)
+ return DurationScale::Hours;
hokein wrote:
> we are comparing with floats, I think we should use some
JDevlieghere added a comment.
In https://reviews.llvm.org/D53263#1294497, @kristina wrote:
> In https://reviews.llvm.org/D53263#1294488, @JDevlieghere wrote:
>
> > The patch applies for me but has quite a few style violations. I'll fix
> > those up before landing it.
>
>
> Thank you and sorry fo
This revision was automatically updated to reflect the committed changes.
Closed by commit rL346601: Pass the function type instead of the return type to
FunctionDecl::Create (authored by JDevlieghere, committed by ).
Herald added a subscriber: llvm-commits.
Changed prior to commit:
https://rev
kristina added a comment.
In https://reviews.llvm.org/D53263#1294488, @JDevlieghere wrote:
> The patch applies for me but has quite a few style violations. I'll fix those
> up before landing it.
Thank you and sorry for the trouble, my fork seems to have enough modifications
to some of these f
Author: jdevlieghere
Date: Sat Nov 10 16:56:15 2018
New Revision: 346601
URL: http://llvm.org/viewvc/llvm-project?rev=346601&view=rev
Log:
Pass the function type instead of the return type to FunctionDecl::Create
Fix places where the return type of a FunctionDecl was being used in
place of the fu
JDevlieghere added a comment.
The patch applies for me but has quite a few style violations. I'll fix those
up before landing it.
https://reviews.llvm.org/D53263
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/
kristina added a comment.
In https://reviews.llvm.org/D53263#1294412, @bobsayshilol wrote:
> In https://reviews.llvm.org/D53263#1294383, @kristina wrote:
>
> > I can do it if you'd like, will be a moment though.
>
>
> Thanks and much appreciated!
Huge apologies, it seems I can't get this to pat
sthibaul added inline comments.
Comment at: lib/Driver/ToolChains/Hurd.cpp:136
+
+ // Add an include of '/include' directly. This isn't provided by default by
+ // system GCCs, but is often used with cross-compiling GCCs, and harmless to
krytarowski wrote:
> Is
kristina updated this revision to Diff 173534.
kristina added a comment.
Revised to address nitpicks.
Repository:
rC Clang
https://reviews.llvm.org/D54344
Files:
lib/CodeGen/CGDeclCXX.cpp
test/CodeGenCXX/attr-no-destroy-d54344.cpp
Index: test/CodeGenCXX/attr-no-destroy-d54344.cpp
=
bobsayshilol added a comment.
In https://reviews.llvm.org/D53263#1294383, @kristina wrote:
> I can do it if you'd like, will be a moment though.
Thanks and much appreciated!
https://reviews.llvm.org/D53263
___
cfe-commits mailing list
cfe-commits
JonasToth added a comment.
Thank you for checking these two projects!
In https://reviews.llvm.org/D53974#1294375, @ztamas wrote:
> I have the result after running the current version of the check on
> LibreOffice.
>
> Found around 50 new issues were hidden by macros:
>
> https://cgit.freedesk
krytarowski added inline comments.
Comment at: lib/Driver/ToolChains/Hurd.cpp:136
+
+ // Add an include of '/include' directly. This isn't provided by default by
+ // system GCCs, but is often used with cross-compiling GCCs, and harmless to
Is this some hurd st
aaron.ballman accepted this revision.
aaron.ballman added a comment.
This revision is now accepted and ready to land.
LGTM aside from some small commenting nits. However, I leave it to
@erik.pilkington to make the final sign-off.
Comment at: lib/CodeGen/CGDeclCXX.cpp:68
+ //
ztamas added a comment.
I also tested on LLVm code.
The output is here:
https://gist.github.com/tzolnai/fe4f23031d3f9fdbdbf1ee38abda00a4
I found 362 warnings.
Around 95% of these warnings are similar to the next example:
/home/zolnai/lohome/llvm/lib/Support/Chrono.cpp:64:24: warning: loop var
kristina added a comment.
In https://reviews.llvm.org/D53263#1294336, @bobsayshilol wrote:
> Thanks!
> I don't have commit access to land this myself.
I can do it if you'd like, will be a moment though.
https://reviews.llvm.org/D53263
___
cfe-co
kristina updated this revision to Diff 173530.
kristina set the repository for this revision to rC Clang.
kristina added a comment.
@erik.pilkington Fantastic catch, I totally forgot about the global flag.
Revised, moved to CodeGenCXX, added a test to verify ctor/dtor is balanced
under normal ci
ztamas added a comment.
I have the result after running the current version if the check.
Found around 50 new issues were hidden by macros:
https://cgit.freedesktop.org/libreoffice/core/commit/?id=afbfe42e63cdba1a18c292d7eb4875009b0f19c0
Also found some new false positives related to macros. The
Author: jvesely
Date: Sat Nov 10 13:43:40 2018
New Revision: 346597
URL: http://llvm.org/viewvc/llvm-project?rev=346597&view=rev
Log:
r600: Add datalayout to image builtin implementation
Signed-off-by: Jan Vesely
Reviewer: Aaron Watry
Modified:
libclc/trunk/r600/lib/image/get_image_attribut
aaron.ballman added a comment.
In https://reviews.llvm.org/D54349#1294325, @vmiklos wrote:
> > What do you think about code like:
> >
> > #if FOO == 4
> > #if FOO == 4
> > #endif
> > #endif
> >
> > #if defined(FOO)
> > #if defined(FOO)
> > #endif
> > #endif
> >
> > #if !d
bobsayshilol added a comment.
Thanks!
I don't have commit access to land this myself.
https://reviews.llvm.org/D53263
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Szelethus added inline comments.
Comment at: clang-tidy/readability/RedundantPreprocessorCheck.cpp:19-22
+struct Entry {
+ SourceLocation Loc;
+ std::string MacroName;
+};
vmiklos wrote:
> Szelethus wrote:
> > This is a way too general name in my opinion. Eithe
vmiklos updated this revision to Diff 173526.
vmiklos marked an inline comment as done.
https://reviews.llvm.org/D54349
Files:
clang-tidy/readability/CMakeLists.txt
clang-tidy/readability/ReadabilityTidyModule.cpp
clang-tidy/readability/RedundantPreprocessorCheck.cpp
clang-tidy/readabilit
vmiklos marked an inline comment as done.
vmiklos added inline comments.
Comment at: clang-tidy/readability/RedundantPreprocessorCheck.cpp:19-22
+struct Entry {
+ SourceLocation Loc;
+ std::string MacroName;
+};
Szelethus wrote:
> This is a way too general name
vmiklos added a comment.
> What do you think about code like:
>
> #if FOO == 4
> #if FOO == 4
> #endif
> #endif
>
> #if defined(FOO)
> #if defined(FOO)
> #endif
> #endif
>
> #if !defined(FOO)
> #if !defined(FOO)
> #endif
> #endif
I looked at supporting these, but it
vmiklos updated this revision to Diff 173524.
https://reviews.llvm.org/D54349
Files:
clang-tidy/readability/CMakeLists.txt
clang-tidy/readability/ReadabilityTidyModule.cpp
clang-tidy/readability/RedundantPreprocessorCheck.cpp
clang-tidy/readability/RedundantPreprocessorCheck.h
docs/Rele
erik.pilkington added a comment.
I have a few nits, but I think this looks about right.
I reduced this testcase with creduce, can you use the minimized version? Also,
I think it makes more sense to put the test into `test/CodeGenCXX` and verify
the `-emit-llvm-only` output is correct with FileC
Szelethus added inline comments.
Comment at: clang-tidy/readability/RedundantPreprocessorCheck.cpp:19-22
+struct Entry {
+ SourceLocation Loc;
+ std::string MacroName;
+};
This is a way too general name in my opinion. Either include comments that
describe it,
Author: rsmith
Date: Sat Nov 10 10:02:40 2018
New Revision: 346591
URL: http://llvm.org/viewvc/llvm-project?rev=346591&view=rev
Log:
[cxx_status] Update for San Diego motions.
Modified:
cfe/trunk/www/cxx_status.html
Modified: cfe/trunk/www/cxx_status.html
URL:
http://llvm.org/viewvc/llvm-pr
JonasToth added a comment.
> Done.
Please also mark the inline comments as done. Otherwise its hard to follow if
there are outstanding issues somewhere, especially if code moves around.
https://reviews.llvm.org/D54349
___
cfe-commits mailing list
Szelethus added a comment.
Mainly my problem is that checker files can be large and hard to maneuver, some
forward declare and document everything, some are a big bowl of spaghetti, and
I think having a checklist of how it should be organized to keep uniformity and
readability would be great.
Szelethus added a comment.
In https://reviews.llvm.org/D52984#1294258, @NoQ wrote:
> In https://reviews.llvm.org/D52984#1294233, @aaron.ballman wrote:
>
> > Personally, I think it's detrimental to the community for subprojects to
> > come up with their own coding guidelines. My preference is for
NoQ added a comment.
In https://reviews.llvm.org/D52984#1294233, @aaron.ballman wrote:
> Personally, I think it's detrimental to the community for subprojects to come
> up with their own coding guidelines. My preference is for the static analyzer
> to come more in line with the rest of the proj
JonasToth added a comment.
In https://reviews.llvm.org/D53771#1294244, @lebedev.ri wrote:
> *Maybe* we should be actually diagnosing the each actual declaration, not
> just the `typeloc`.
> Else if you use one `typedef`, and `// NOLINT` it, you will silence it for
> all the decls that use it.
sthibaul updated this revision to Diff 173515.
https://reviews.llvm.org/D54379
Files:
lib/Basic/Targets.cpp
lib/Basic/Targets/OSTargets.h
lib/Driver/CMakeLists.txt
lib/Driver/Driver.cpp
lib/Driver/ToolChains/Clang.cpp
lib/Driver/ToolChains/Gnu.cpp
lib/Driver/ToolChains/Hurd.cpp
li
sthibaul marked 3 inline comments as done.
sthibaul added a comment.
I commented one of them, and will fix the rest.
Comment at: lib/Driver/ToolChains/Hurd.cpp:36
+ // clever.
+ switch (TargetTriple.getArch()) {
+ default:
kristina wrote:
> Does this need a
lebedev.ri added a comment.
*Maybe* we should be actually diagnosing the each actual declaration, not just
the `typeloc`.
Else if you use one `typedef`, and `// NOLINT` it, you will silence it for all
the decls that use it.
In https://reviews.llvm.org/D53771#1294241, @JonasToth wrote:
> LGTM.
JonasToth accepted this revision.
JonasToth added a comment.
This revision is now accepted and ready to land.
LGTM.
Please give other reviewers a chance to take a look at it too.
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D53771
lebedev.ri added inline comments.
Comment at: test/clang-tidy/modernize-avoid-c-arrays.cpp:32
+
+using k = int[4];
+// CHECK-MESSAGES: :[[@LINE-1]]:11: warning: do not declare C-style arrays,
use std::array<> instead
JonasToth wrote:
> What happens for a variabl
kristina added reviewers: kristina, clang.
kristina added a comment.
A few style naming/comments.
Comment at: lib/Driver/ToolChains/Hurd.cpp:36
+ // clever.
+ switch (TargetTriple.getArch()) {
+ default:
Does this need a switch? Wouldn't an `if` be sufficien
aaron.ballman added a comment.
In https://reviews.llvm.org/D52984#1294223, @xazax.hun wrote:
> - Move the checklist up before additional info in the HTML file.
> - Fix minor nits.
> - Add missing bullet points (thanks @Szelethus for noticing)
>
> I did not add any coding convention related item
JonasToth added a comment.
from my side mostly ok. I think the typedef points should be clarified, but I
dont see more issues.
Comment at: test/clang-tidy/modernize-avoid-c-arrays.cpp:32
+
+using k = int[4];
+// CHECK-MESSAGES: :[[@LINE-1]]:11: warning: do not declare C-style
lebedev.ri added inline comments.
Comment at: clang-tidy/modernize/AvoidCArraysCheck.cpp:64
+ diag(ArrayType->getBeginLoc(),
+ isa(ArrayType->getTypePtr()) ? UseVector : UseArray);
+}
JonasToth wrote:
> Why `isa<>` and not `isVariableArrayType()` (does it
lebedev.ri updated this revision to Diff 173514.
lebedev.ri marked 2 inline comments as done.
lebedev.ri added a comment.
Use `isVariableArrayType()`
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D53771
Files:
clang-tidy/cppcoreguidelines/CMakeLists.txt
clang-tidy/cppcoregu
xazax.hun added inline comments.
Comment at: www/analyzer/checker_dev_manual.html:719
+
+User facing documentation is important for adoption! Make sure the check
list updated
+at the homepage of the analyzer. Also ensure that the description is good
quality in
-
xazax.hun updated this revision to Diff 173513.
xazax.hun marked 11 inline comments as done.
xazax.hun added a comment.
- Move the checklist up before additional info in the HTML file.
- Fix minor nits.
- Add missing bullet points (thanks @Szelethus for noticing)
I did not add any coding conventi
Szelethus added inline comments.
Comment at: www/analyzer/checker_dev_manual.html:719
+
+User facing documentation is important for adoption! Make sure the check
list updated
+at the homepage of the analyzer. Also ensure that the description is good
quality in
-
xazax.hun added inline comments.
Comment at: www/analyzer/checker_dev_manual.html:719
+
+User facing documentation is important for adoption! Make sure the check
list updated
+at the homepage of the analyzer. Also ensure that the description is good
quality in
-
kristina added inline comments.
Comment at: lib/Driver/ToolChains/Clang.cpp:533
- if (Triple.isOSLinux() || Triple.getOS() == llvm::Triple::CloudABI) {
+ if (Triple.isOSLinux() || Triple.getOS() == llvm::Triple::CloudABI ||
Triple.isOSHurd()) {
switch (Triple.getArch())
sthibaul added a comment.
The Hurd::Hurd constructor would actually need to do the same gcc inclusion
path detection as on Linux, but let's leave this aside for now, this commit is
enough for a build without libc++.
Repository:
rC Clang
https://reviews.llvm.org/D54379
___
sthibaul created this revision.
sthibaul added a reviewer: rengolin.
Herald added subscribers: cfe-commits, fedor.sergeev, krytarowski, mgorny.
Repository:
rC Clang
https://reviews.llvm.org/D54379
Files:
lib/Basic/Targets.cpp
lib/Basic/Targets/OSTargets.h
lib/Driver/CMakeLists.txt
lib/
JonasToth added inline comments.
Comment at: clang-tidy/modernize/AvoidCArraysCheck.cpp:26
+ const clang::Type *TypeNode = Node.getTypePtr();
+ return (TypeNode != nullptr &&
+ InnerMatcher.matches(*TypeNode, Finder, Builder));
Nit: these parens are su
lebedev.ri added a comment.
Comments? :)
https://reviews.llvm.org/D53771
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
mgorny added a comment.
Merged. I will get back to you if something explodes ;-).
Repository:
rL LLVM
https://reviews.llvm.org/D54120
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commi
Author: mgorny
Date: Sat Nov 10 03:41:36 2018
New Revision: 346586
URL: http://llvm.org/viewvc/llvm-project?rev=346586&view=rev
Log:
[python] Support PathLike filenames and directories
Python 3.6 introduced a file system path protocol (PEP 519[1]).
The standard library APIs accepting file system
This revision was automatically updated to reflect the committed changes.
Closed by commit rL346586: [python] Support PathLike filenames and directories
(authored by mgorny, committed by ).
Herald added a subscriber: llvm-commits.
Changed prior to commit:
https://reviews.llvm.org/D54120?vs=1735
kristina updated this revision to Diff 173507.
kristina added a comment.
Revised, added a newline to the testcase.
https://reviews.llvm.org/D54344
Files:
lib/CodeGen/CGDeclCXX.cpp
test/SemaCXX/attr-no-destroy-d54344.cpp
Index: test/SemaCXX/attr-no-destroy-d54344.cpp
===
JonasToth added a comment.
> I'll run it on LibreOffice code again and we'll see.
Perfect, if you have the time, running it against LLVM would be very
interesting as well.
>> Do you have commit access?
>
> Commit access? This is my first patch.
If you plan to regularly contribute to LLVM you
lebedev.ri added inline comments.
Comment at: test/SemaCXX/attr-no-destroy-d54344.cpp:93
+static some_class s_ctor(1);
\ No newline at end of file
Please add new line.
Repository:
rC Clang
https://reviews.llvm.org/D54344
_
kristina updated this revision to Diff 173503.
kristina set the repository for this revision to rC Clang.
kristina added a comment.
Revised, I think I worked out what was happening, there's an explanation in the
comment in the test. This is a relatively new attribute so the coverage for it
did n
jstasiak updated this revision to Diff 173505.
jstasiak added a comment.
That's fair, changed string to just x, should be obvious from the context what
x is.
Thank you for the review. As I don't have commit access I'd like to ask you to
commit this on my behalf.
https://reviews.llvm.org/D5412
mgorny accepted this revision.
mgorny added a comment.
This revision is now accepted and ready to land.
LGTM, thanks!
Comment at: bindings/python/clang/cindex.py:133
+except AttributeError:
+def fspath(string):
+return string
Optionally: this is now
mgorny requested changes to this revision.
mgorny added a comment.
This revision now requires changes to proceed.
Could you please rebase? I'm pretty sure this breaks our use but I can't test
since it no longer applies cleanly.
https://reviews.llvm.org/D32595
mgorny accepted this revision.
mgorny added a comment.
This revision is now accepted and ready to land.
LGTM.
https://reviews.llvm.org/D32578
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-
Szelethus added a comment.
Ping, @NoQ, do you insist on a global set?
https://reviews.llvm.org/D51531
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
kristina added a comment.
I've managed to isolate enough to make for a testcase. Is that too broad or is
it okay to attach as is?
The following triggers the assert:
namespace util {
template
class test_vector
{
public:
test_vector(unsigned c)
mgorny accepted this revision.
mgorny added a comment.
This revision is now accepted and ready to land.
LGTM
Repository:
rC Clang
https://reviews.llvm.org/D32577
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bi
jstasiak marked 2 inline comments as done.
jstasiak added a comment.
Thanks for the feedback, you're right those were things worth improving, I
updated the code.
https://reviews.llvm.org/D54120
___
cfe-commits mailing list
cfe-commits@lists.llvm.or
jstasiak updated this revision to Diff 173496.
jstasiak added a comment.
Tests are skipped using unittest skipping mechanism now and pathlib imported
only when necessary.
https://reviews.llvm.org/D54120
Files:
bindings/python/clang/cindex.py
bindings/python/tests/cindex/test_cdb.py
bindi
mgorny requested changes to this revision.
mgorny added a comment.
This revision now requires changes to proceed.
Also please remember to submit patches with `-U`, so that Phab has full
context.
Comment at: bindings/python/tests/cindex/test_cdb.py:42
+if HAS_FSPATH:
Author: kristina
Date: Sat Nov 10 00:04:38 2018
New Revision: 346583
URL: http://llvm.org/viewvc/llvm-project?rev=346583&view=rev
Log:
[clang]: Fix misapplied patch in 346582.
Modified:
cfe/trunk/lib/CodeGen/CGDeclCXX.cpp
Modified: cfe/trunk/lib/CodeGen/CGDeclCXX.cpp
URL:
http://llvm.org/v
73 matches
Mail list logo
