efriedma added inline comments.
Comment at: lib/Basic/Targets/X86.h:898
+ MaxAtomicPromoteWidth = 64;
+ MaxAtomicInlineWidth = 64;
+}
wmi wrote:
> efriedma wrote:
> > I don't think we need to mess with MaxAtomicPromoteWidth?
> >
> > Probably more i
efriedma added inline comments.
Comment at: lib/Sema/SemaChecking.cpp:8176
+return IntRange(C.getIntWidth(QualType(T, 0)),
+!ET->isSignedIntegerOrEnumerationType());
Maybe add a comment noting what can trigger this case?
==
efriedma accepted this revision.
efriedma added a comment.
This revision is now accepted and ready to land.
LGTM
Repository:
rL LLVM
https://reviews.llvm.org/D38046
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi
efriedma added inline comments.
Comment at: lib/Sema/SemaChecking.cpp:8176
+ // underlying type, so that when someone specifies the type as
+ // "unsigned" it doesn't cause sign-conversion type warnings.
if (!Enum->isCompleteDefinition())
Explici
efriedma accepted this revision.
efriedma added a comment.
This revision is now accepted and ready to land.
LGTM
https://reviews.llvm.org/D38145
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/c
efriedma accepted this revision.
efriedma added a comment.
This revision is now accepted and ready to land.
LGTM.
Do you want me to commit this for you?
https://reviews.llvm.org/D37861
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://
efriedma added subscribers: cfe-commits, efriedma.
efriedma added a comment.
Please make sure to add the mailing list as a subscriber when you post a patch.
(I haven't looked at the patch yet.)
https://reviews.llvm.org/D38126
___
cfe-commits maili
efriedma added a comment.
> Should I restrict the warning to just redeclarations (without definition)
> instead?
I think just redeclarations. The pattern of a declaration without a section
followed by a definition with a section is common, and not really harmful.
Comment at
efriedma added inline comments.
Comment at: clang/lib/AST/ExprConstant.cpp:9457
+ LHS = APSInt(LHS.isSigned() ? LHS.sextOrSelf(MaxBits)
+ : LHS.zextOrSelf(MaxBits),
!IsSigned);
Can you just write `LHS = AP
efriedma accepted this revision.
efriedma added a comment.
This revision is now accepted and ready to land.
LGTM
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D62665/new/
https://reviews.llvm.org/D62665
___
cfe-commits mailing list
cfe-commi
efriedma added a comment.
Something I ran into when reviewing https://reviews.llvm.org/D62639 is that on
AArch64, for varargs functions, we emit floating-point stores when
noimplicitfloat is specified. That seems fine for -mno-implicit-float, but
maybe not for -mgeneral-regs-only?
CHANGES SI
efriedma added a comment.
The general idea of restricting the intrinsics to specific contexts makes
sense. I'm not sure it makes sense to mark expressions, as opposed to types,
though; can we really expect the user to know which expressions to apply this
to?
I'd like to see an actual specific
efriedma added a comment.
For format-strings.c, I'm not really happy suggesting `#if defined(__sun) &&
!defined(__LP64__)`, but I don't think the alternative is better. We could
restrict the test so it doesn't run using a Solaris target triple, but we
actually want coverage here: the differenc
efriedma added inline comments.
Comment at: test/Sema/address_spaces.c:12
{
- _AS2 *x;// expected-warning {{type specifier missing, defaults to 'int'}}
+ _AS2 *x;// expected-error {{use of undeclared identifier 'x'}}
_AS1 float * _AS2 *B;
xbolva00 wrote:
>
efriedma added inline comments.
Comment at: test/Sema/address_spaces.c:12
{
- _AS2 *x;// expected-warning {{type specifier missing, defaults to 'int'}}
+ _AS2 *x;// expected-error {{use of undeclared identifier 'x'}}
_AS1 float * _AS2 *B;
xbolva00 wrote:
>
efriedma accepted this revision.
efriedma added a comment.
This revision is now accepted and ready to land.
LGTM
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D61750/new/
https://reviews.llvm.org/D61750
___
cfe-commits mailing list
cfe-commi
efriedma accepted this revision.
efriedma added a comment.
This revision is now accepted and ready to land.
LGTM
Repository:
rC Clang
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D62944/new/
https://reviews.llvm.org/D62944
___
cfe-commit
efriedma added inline comments.
Comment at: lib/CodeGen/CGExprConstant.cpp:967
// Constant folding is currently missing support for a few features supported
// here: CK_ToUnion, CK_ReinterpretMemberPointer, and DesignatedInitUpdateExpr.
class ConstExprEmitter :
---
efriedma added a comment.
> Now, we could theoretically use a different ABI rule for vectors defined with
> Clang-specific extensions, but that seems like it would cause quite a few
> problems of its own.
I think we can't reasonably impose this ABI rule on vectors defined with
ext_vector_type:
efriedma added a comment.
> Are you saying that using MMX in LLVM requires source-level workarounds in
> some way, and so we can't lower portable code to use MMX because that code
> will (reasonably) lack those workarounds?
Yes.
The x86 architecture requires that a program executes an "emms" i
efriedma added a comment.
If we're going to insert emms instructions automatically, it doesn't really
make sense to do it in the frontend; the backend could figure out the most
efficient placement itself. (See lib/Target/X86/X86VZeroUpper.cpp, which
implements similar logic for AVX.) The part
efriedma added a comment.
> I'm just laying out the basic requirements for getting this patch back in,
> because the current patch is invalid given LLVM's current requirements.
Yes, I'm on the same page.
Repository:
rL LLVM
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D59744/new/
h
efriedma added inline comments.
Comment at: clang/lib/Parse/ParseStmt.cpp:104
+
+ if (isNullStmtWithAttributes()) {
+MaybeParseGNUAttributes(Attrs);
If we're going to generally support statement attributes, it should be possible
to apply them to non-null st
efriedma added a comment.
Yes, that seems reasonable.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D64838/new/
https://reviews.llvm.org/D64838
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
ht
efriedma edited reviewers, added: aaron.ballman; removed: doug.gregor,
eli.friedman, lvoufo.
efriedma added a comment.
Is this the only place where a compound type can contain a PackExpansionType?
The code could probably use a brief comment explaining why we need to check
this explicitly, even
efriedma added inline comments.
Comment at: clang/lib/CodeGen/CGStmt.cpp:2287
assert(RegResults.size() == ResultRegDests.size());
+ assert(ResultTypeRequiresCast.size() <= ResultRegDests.size());
for (unsigned i = 0, e = RegResults.size(); i != e; ++i) {
N
efriedma added inline comments.
Comment at: clang/test/CodeGen/PR42672.c:50
+ struct {
+long long int v1, v2, v3, v4;
+ } str;
glider wrote:
> The acceptable size actually depends on the target platform. Not sure we can
> test for all of them, and we'll pr
efriedma added inline comments.
Comment at: lib/CodeGen/CGBuiltin.cpp:8182
switch (BuiltinID) {
default: return nullptr;
case NEON::BI__builtin_neon_vbsl_v:
I'm a little concerned about the overall code structure here; even if moving
the code for the MS
efriedma accepted this revision.
efriedma added a comment.
This revision is now accepted and ready to land.
LGTM
Comment at: lib/CodeGen/CGBuiltin.cpp:8182
switch (BuiltinID) {
default: return nullptr;
case NEON::BI__builtin_neon_vbsl_v:
dmajor wrote:
efriedma added a comment.
> this looks like it could be a Core Issue
I think the issue is clang-specific. clang splits the standard notion of a
dependent type into two separate bits, for the sake of diagnostics:
`isDependentType()`, and `isInstantiationDependentType()`.
`isInstantiationDepend
efriedma accepted this revision.
efriedma added a comment.
This revision is now accepted and ready to land.
LGTM
It would be cleaner to convert this test to FileCheck, but it's probably not
worth spending the time at this point.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
efriedma added a comment.
For the "=x" thing I was talking about, try the following testcase:
void a() { struct S { unsigned x[4]; } z; asm volatile("%0":"=x"(z)); }
void a2() { struct S { unsigned x[8]; } z; asm volatile("%0":"=x"(z)); }
clang trunk gives "error: couldn't allocate output re
efriedma added a comment.
This seems better.
I'm not sure I follow why this needs special handling in
SelectionDAGBuilder::visitIntrinsicCall, as opposed to just using
ISD::INTRINSIC_VOID like other similar target-specific intrinsics. (You can
custom-lower INTRINSIC_VOID in ARMTargetLowering:
efriedma added a comment.
Yes, it's technically a "call", but you don't need the call lowering code.
That's dedicated to stuff like passing arguments, returning values, checking
whether the function can be tail-called, etc. None of that applies here; the
intrinsic always corresponds to exactl
efriedma added a comment.
> It seems global-isel does not fall back to DAGISel?
It does, for targets where it's enabled by default, or if you use the right
flags. I think you want `-global-isel -global-isel-abort=2`?
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://r
efriedma added inline comments.
Comment at: llvm/lib/Target/ARM/ARMExpandPseudoInsts.cpp:1927
+.add(predOps(ARMCC::AL))
+.addReg(ARM::LR);
+
I think you need to ensure that lr actually contains the correct value,
somehow. Normally the ca
efriedma accepted this revision.
efriedma added a comment.
This revision is now accepted and ready to land.
LGTM
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D65234/new/
https://reviews.llvm.org/D65234
___
efriedma added a comment.
This might be a silly question, but what happens if the initializer for a
thread-local variable refers to another thread-local variable? Do you need to
initialize both variables? In what order?
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https
efriedma added a comment.
> If variable A's initializer references variable B, then it will call B's
> initializer.
I'm considering a testcase like this:
struct S { int x; };
void bar(S**);
void baz(void());
void f() {
thread_local S s = {1};
thread_local S* p = &s;
b
efriedma added a comment.
> in the proper order
I would prefer lexical order, if possible. (At least, the order should be
deterministic.)
> clang should either generate an error or do "the right thing."
Agreed.
I think we should send a defect report to the C++ standards committee to
clarify
efriedma added a comment.
The new approach to tracking expressions inside of
__builtin_preserve_access_index seems okay.
Please let @rsmith comment since he looked at this before.
Comment at: docs/LanguageExtensions.rst:1958
+array subscript access and structure/union member
efriedma added a comment.
> Since this is an extension, it wouldb be great to have a (on-by-default? at
> least in -Wall) diagnostic for every instance of usage of this extension.
We generally only add warnings for extensions which are not allowed by the
standard. It's impossible to define a s
efriedma added a comment.
I don't think this transform is valid, for the same reasons we don't do it in
IR optimizations.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D64128/new/
https://reviews.llvm.org/D64128
_
efriedma added a comment.
Do the changes to BuiltinsARM.def have any practical effect? long should be 32
bits on all 32-bit ARM targets.
Repository:
rC Clang
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D64164/new/
https://reviews.llvm.org/D64164
efriedma added a comment.
> If they're all syntactically together like this, maybe that's safe?
Having them together syntactically doesn't really help, I think; it might be
guarded by some code that does the same conversion (and if you repeat the
conversion, it has to produce the same result).
efriedma added a comment.
> -fno-builtin* is about preventing clang/llvm from recognizing that a piece of
> code has the same semantic as a particular IR intrinsic, it has nothing to do
> with preventing the compiler from generating runtime calls.
It has a dual purpose for C library functions.
efriedma added a comment.
> The size you allocate here will presumably need to vary as the struct layout
> changes, and you have no way of knowing which allocas will need their sizes
> to be changed.
Your example is just a pointer; the size of a pointer won't change.
That said, yes, address co
efriedma added a comment.
My main blocker is that I want to make sure we're moving in the right
direction: towards LLVM IR with clear semantics, towards straightforward rules
for writing freestanding C code, and towards solutions which behave
appropriately for all targets. There's clearly a pr
efriedma added a comment.
Adding "Z" makes sense.
If you're going to mess with the 64-bit builtins, it's probably worth adding a
testcase that can be built with gcc to show that int64_t is actually correct.
You should be able to write a C++ testcase using decltype (declare a variable
of type
efriedma added inline comments.
Comment at: test/CodeGen/avr-builtins.c:1
+// REQUIRES: avr-registered-target
+// RUN: %clang_cc1 -triple avr-unknown-unknown -emit-llvm -o - %s | FileCheck
%s
I don't think this needs avr-registered-target; it doesn't actually in
efriedma added a comment.
"complex" in this context is the C99 `_Complex`, which is supported in C++ as
an extension, using the same syntax as C. You can just declare a variable with
type `_Complex float`. If you need to manipulate the real and imaginary parts
of a variable, you can use the gc
efriedma added a comment.
> I have a related patch that turns -fno-builtin* options into module flags
Do you have any opinion on representing -fno-builtin using a module flag vs. a
function attribute in IR? It seems generally more flexible and easier to
reason about a function attribute from m
efriedma added inline comments.
Comment at: test/CodeGen/builtins.cpp:5
+// RUN: %clang_cc1 -std=c++11 -triple powerpc-pc-linux -verify %s
+// RUN: %clang_cc1 -std=c++11 -triple arm-linux-gnueabi -verify %s
+
You don't need quite so many targets on this list. Th
efriedma added a comment.
> If we agree that this is a good way forward, there also appears to be
> +neonfp/-neonfp additions happening in handleTargetFeatures that should
> probably be happening in initFeatureMap instead?
neonfp isn't passed as a feature in the first place; there's a separ
efriedma accepted this revision.
efriedma added a comment.
This revision is now accepted and ready to land.
LGTM
Repository:
rC Clang
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D61845/new/
https://reviews.llvm.org/D61845
___
cfe-commit
efriedma added inline comments.
Comment at: lib/CodeGen/CGExpr.cpp:3359
+// Remember the original array subscript for bpf target
+unsigned idx = cast(indices.back())->getZExtValue();
+eltPtr = CGF.Builder.CreatePreserveArrayAccessIndex(addr.getPointer(),
-
efriedma added inline comments.
Comment at: lib/Headers/__clang_cuda_cmath.h:55-56
+#if defined(_OPENMP) && defined(__cplusplus)
+__DEVICE__ const float abs(const float __x) { return ::fabsf((float)__x); }
+__DEVICE__ const double abs(const double __x) { return ::fabs((double)__x
efriedma added a comment.
Please verify my understanding of the following is correct:
1. getTypeUnadjustedAlign() is currently only used to compute calling
convention alignment for ARM and AArch64.
2. Without this patch, we use the unadjusted alignment to pass arguments, but
the adjusted alignm
efriedma accepted this revision.
efriedma added a comment.
This revision is now accepted and ready to land.
Oh, I see, ABIInfo::computeInfo() uses the canonical type to compute the
calling convention, and that throws away alignment attributes because alignment
attributes on non-struct are broken
efriedma added a comment.
We don't necessarily need to block the clang changes on the backend error
reporting actually being implemented, I guess, if the architecture we want is
settled.
With this patch, do we pass the general-regs-only attribute to the backend? If
so, would that be the attri
efriedma added inline comments.
Comment at: clang/lib/Sema/SemaExprCXX.cpp:7951
+ // they don't have to write out memcpy() for simple cases. For that reason,
+ // it's very limited in what it will detect.
+ //
We don't always lower struct copies to memcpy(); I
efriedma accepted this revision.
efriedma added a comment.
LGTM
Repository:
rC Clang
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D58375/new/
https://reviews.llvm.org/D58375
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https:/
efriedma added inline comments.
Comment at: clang/lib/Sema/SemaExprCXX.cpp:8003
+!isRValueOfIllegalType(E) &&
+E->isConstantInitializer(S.getASTContext(), /*ForRef=*/false)) {
+ resetDiagnosticState(InitialState);
george.burgess.iv wrote:
> e
efriedma added a comment.
Are the behavior differences between the newpm alwaysinliner and the oldpm
alwaysinliner intentional? Specifically, the differences in pass remarks, and
the differences in the treatment of the alwaysinline attribute seem suspect.
(I'm not that interested in the diffe
efriedma added inline comments.
Comment at: lib/CodeGen/CGStmt.cpp:1825
+bool Success = false;
+if (CGM.getCodeGenOpts().OptimizationLevel > 0)
+ Success = InputExpr->EvaluateAsInt(Result, getContext());
Checking the optimization level here doesn't m
efriedma added inline comments.
Comment at: test/CodeGen/builtin-constant-p.c:2
+// RUN: %clang_cc1 -triple x86_64-unknown-unknown -emit-llvm -o - %s -O2 |
FileCheck --check-prefix=O2 %s
+// RUN: %clang_cc1 -triple x86_64-unknown-unknown -emit-llvm -o - %s -O0 |
FileCheck --che
efriedma accepted this revision.
efriedma added a comment.
This revision is now accepted and ready to land.
LGTM with a minor nit, but give a couple days for @jyknight to add any more
comments.
Comment at: test/CodeGen/builtin-constant-p.c:2
+// RUN: %clang_cc1 -triple x86_64-
efriedma added a comment.
This looks reasonable.
Comment at: clang/lib/CodeGen/CGBuiltin.cpp:1265
+NeedSExt = Op1Info.Signed;
+NeedZExt = !Op1Info.Signed;
+ } else if (Op1Info.Width > Op2Info.Width) {
This is a weird way to express this... could you in
efriedma added inline comments.
Comment at: test/CodeGen/builtin-constant-p.c:2
+// RUN: %clang_cc1 -triple x86_64-unknown-unknown -emit-llvm -o - %s -O2 |
FileCheck --check-prefix=O2 %s
+// RUN: %clang_cc1 -triple x86_64-unknown-unknown -emit-llvm -o - %s -O0 |
FileCheck --che
efriedma accepted this revision.
efriedma added a comment.
This revision is now accepted and ready to land.
LGTM
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D55843/new/
https://reviews.llvm.org/D55843
___
cfe-commits mailing list
cfe-commi
efriedma added inline comments.
Comment at: lib/Sema/SemaChecking.cpp:12381
+// It is possible that the base type is incomplete (see PR39746), even
+// though the effective type is complete. In this case we have no info
+// about the size of the base type and so skip
efriedma added inline comments.
Comment at: lib/Sema/SemaChecking.cpp:12357
+ Context.getAsConstantArrayType(BaseExpr->getType());
+ const Type *BaseType = BaseExpr->getType()->getPointeeOrArrayElementType();
+
Using getPointeeOrArrayElementType here is kin
efriedma accepted this revision.
efriedma added a comment.
This revision is now accepted and ready to land.
LGTM
Repository:
rC Clang
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D55862/new/
https://reviews.llvm.org/D55862
___
cfe-commit
efriedma accepted this revision.
efriedma added a comment.
This revision is now accepted and ready to land.
LGTM
Repository:
rC Clang
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D56050/new/
https://reviews.llvm.org/D56050
___
cfe-commit
efriedma added a comment.
It's currently possible to write, in clang:
void f() {
__try {
}
__except(({__try{}__finally{}; 3;})) {
}
}
And the following currently crashes if you try to build it with clang:
struct A { ~A(); };
int f(const A&);
void g() {
__try {
efriedma added a comment.
Missing changes to lib/Analysis/CFG.cpp.
Comment at: lib/Sema/SemaStmtAsm.cpp:470
+if (NS->isGCCAsmGoto() &&
+Exprs[ConstraintIdx]->getStmtClass() == Stmt::AddrLabelExprClass)
+ break;
This looks suspicious; an AddrLab
efriedma created this revision.
efriedma added a reviewer: ostannard.
Herald added a subscriber: kristof.beyls.
Herald added a project: clang.
Matching fix for https://reviews.llvm.org/D67375 .
Repository:
rC Clang
https://reviews.llvm.org/D67467
Files:
lib/Basic/Targets/ARM.cpp
lib/Driv
This revision was automatically updated to reflect the committed changes.
Closed by commit rL372187: [ARM] Update clang for removal of vfp2d16 and
vfp2d16sp (authored by efriedma, committed by ).
Herald added a project: LLVM.
Herald added a subscriber: llvm-commits.
Changed prior to commit:
htt
efriedma added a subscriber: rsmith.
efriedma added a comment.
Does this have any significant impact on -fsyntax-only performance?
Hopefully @rsmith can take a quick look at the use of ConstantExpr here; I
think it's fine, but we don't use ConstantExpr like that elsewhere.
Co
This revision was automatically updated to reflect the committed changes.
Closed by commit rL329965: Remove -cc1 option "-backend-option".
(authored by efriedma, committed by ).
Herald added a subscriber: llvm-commits.
Changed prior to commit:
https://reviews.llvm.org/D45109?vs=140484&id=142277
efriedma added a subscriber: pcc.
efriedma added a comment.
The test wasn't checking anything useful; the clang driver never passes
"-arm-long-calls" to clang -cc1. See https://reviews.llvm.org/rL241565 .
-Eli
Repository:
rL LLVM
https://reviews.llvm.org/D45109
_
efriedma added a comment.
This makes sense.
Comment at: include/clang/Frontend/Utils.h:241
+/// then the value of this boolean will be true, otherwise false.
+extern bool FrontendTimesIsEnabled;
+
Don't really like global variables, but I guess timers are globa
efriedma added a comment.
> The fcmp opcode has no defined behavior with NaN operands in the comparisions
> handled in this patch.
Could you describe the problem here in a bit more detail? As far as I know,
the LLVM IR fcmp should return the same result as the x86 CMPPD, even without
fast-mat
efriedma added a comment.
We can could add an exception to the "don't allow redeclarations with custom
type-checking" rule to specifically allow redeclaring `__va_start`. The
general rule is that we don't want user code redeclaring them because they
don't have a specific signature, so our inte
efriedma added a comment.
I don't see an `A` in `LANGBUILTIN(__va_start, "vc**.", "nt",
ALL_MS_LANGUAGES)`
https://reviews.llvm.org/D45383
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/
efriedma added inline comments.
Comment at: lib/Basic/FrontendTiming.cpp:18
+
+bool FrontendTimesIsEnabled = false;
+
avt77 wrote:
> efriedma wrote:
> > Why is this in lib/Basic, when the declaration is in
> > include/clang/Frontend/?
> Because this library is b
efriedma added a comment.
It looks like you didn't fix the tests?
https://reviews.llvm.org/D45383
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
efriedma accepted this revision.
efriedma added a comment.
This revision is now accepted and ready to land.
LGTM
https://reviews.llvm.org/D45383
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/c
efriedma closed this revision.
efriedma added a comment.
https://reviews.llvm.org/rL330162
Repository:
rCXXA libc++abi
https://reviews.llvm.org/D45196
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/l
This revision was automatically updated to reflect the committed changes.
Closed by commit rL330169: [ARM] Compute a target feature which corresponds to
the ARM version. (authored by efriedma, committed by ).
Herald added a subscriber: llvm-commits.
Changed prior to commit:
https://reviews.llvm
efriedma created this revision.
efriedma added a reviewer: rsmith.
WIP because I'm having trouble figuring out where to diagnose friend templates;
suggestions welcome.
Repository:
rC Clang
https://reviews.llvm.org/D45712
Files:
lib/Sema/SemaDeclCXX.cpp
test/CXX/class.access/class.friend
efriedma added inline comments.
Comment at: lib/Sema/SemaChecking.cpp:2623
+QualType QT = Pointer->getType()->getPointeeType();
+if (!QT.isNull() && QT->isBooleanType())
+ // Warn on bool* to bool conversion.
Have
efriedma added a comment.
The thing about the bool*-only version is that bool pointers are rare in C++,
so I'm not sure we're gaining much. But if we can't do something more general,
there's still some benefit.
I see your point about false positives for the more general version. I was sort
of
efriedma added inline comments.
Comment at: lib/Frontend/FrontendTiming.cpp:14
+
+#include "llvm/Support/Timer.h"
+
This should include clang/Frontend/Utils.h .
Comment at: test/Frontend/ftime-report-template-decl.cpp:2
+// RUN: %clang %s -S -o
efriedma added a comment.
The list of features/extensions seems okay; it's information which is already
available through a stable interface (specifically, clang -E). JSON also seems
fine as a representation.
Including the language/codegen/etc. options as-is doesn't make sense; we can,
and of
efriedma added a comment.
> If any of those options we care about wind up being changed, there's a good
> chance we may need to change something on our end anyway, so breaking us is
> actually useful.
I'm not sure I follow. The language options have specific consequences you
could check some
efriedma accepted this revision.
efriedma added a comment.
This revision is now accepted and ready to land.
LGTM
https://reviews.llvm.org/D45619
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/c
efriedma created this revision.
efriedma added reviewers: fhahn, SjoerdMeijer.
Herald added a reviewer: javed.absar.
Herald added a subscriber: kristof.beyls.
Passing the features in random order will lead to unpredictable results when
some of the features are related (like the architecture-versi
efriedma updated this revision to Diff 143821.
efriedma added a comment.
Add REQUIRES line to testcase.
Repository:
rC Clang
https://reviews.llvm.org/D46030
Files:
lib/Basic/Targets.cpp
test/CodeGen/arm-build-attributes.c
Index: test/CodeGen/arm-build-attributes.c
=
This revision was automatically updated to reflect the committed changes.
Closed by commit rL330861: [TargetInfo] Sort target features before passing
them to the backend (authored by efriedma, committed by ).
Herald added a subscriber: llvm-commits.
Changed prior to commit:
https://reviews.llvm
301 - 400 of 1777 matches
Mail list logo
