rsmith added a comment.
In https://reviews.llvm.org/D29753#688834, @bruno wrote:
> > It seems to me that the problem here is that `DeclMustBeEmitted` is not
> > safe to call in the middle of deserialization if anything
> > partially-deserialized might be reachable from the declaration we're
>
rsmith accepted this revision.
rsmith added a comment.
This revision is now accepted and ready to land.
Hal and I discussed exactly the same problem (in the context of coroutines) on
Saturday and came up with exactly the same approach :) I think this is the
right direction.
C
rsmith accepted this revision.
rsmith added inline comments.
This revision is now accepted and ready to land.
Comment at: include/clang/Sema/ScopeInfo.h:138-140
+ /// \brief Whether this function has already built, or tried to build, the
+ /// the initial and final coroutine su
rsmith added inline comments.
Comment at: lib/Sema/TreeTransform.h:6802
+ return getDerived().RebuildDependentCoawaitExpr(
+ E->getKeywordLoc(), Result.get(), E->getOperatorCoawaitLookup());
+}
EricWF wrote:
> rsmith wrote:
> > You need to transform the Unr
rsmith added inline comments.
Comment at: test/CXX/drs/dr3xx.cpp:911
-namespace dr373 { // dr373: no
- // FIXME: This is valid.
- namespace X { int dr373; } // expected-note 2{{here}}
+namespace dr373 { // dr373: yes
+ namespace X { int dr373; }
This should
rsmith added a comment.
Functionally, this looks good. How do the diagnostics look in the case where
lookup only finds a non-namespace name? Eg,
struct A { struct B {}; };
namespace X = A::B;
https://reviews.llvm.org/D30848
___
cfe-commits mai
rsmith added inline comments.
Comment at: lib/Sema/Sema.cpp:472-477
+// If this is a function template, we should remove if it has no
+// specializations.
+if (FunctionTemplateDecl *Template = FD->getDescribedFunctionTemplate()) {
+ if (std::distance(Template->sp
rsmith added a comment.
Do we still need this after https://reviews.llvm.org/D23765 lands?
https://reviews.llvm.org/D27116
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
rsmith added a comment.
A target-specific default for this, simply because there's a lot of code on
Darwin that happens to violate this language rule, doesn't make sense to me.
Basing the behavior on whether a `-Wreturn-type` warning would have been
emitted seems like an extremely strange heuri
rsmith added a comment.
In https://reviews.llvm.org/D27163#607078, @rsmith wrote:
> A target-specific default for this, simply because there's a lot of code on
> Darwin that happens to violate this language rule, doesn't make sense to me.
... but rjmccall's explanation of the problem helps. Th
rsmith added inline comments.
Comment at: lib/Frontend/FrontendActions.cpp:227
+IsBuiltin = true;
+ addHeaderInclude(H.NameAsWritten, Includes, LangOpts, Module->IsExternC,
+ IsBuiltin /*AlwaysInclude*/);
I don't understand why
rsmith added inline comments.
Comment at: include/clang/Basic/DiagnosticSemaKinds.td:4556-4558
+ "%0 is a %select{non-tag type|typedef|type alias|template|type alias "
+ "template|template template argument}1 that cannot be referenced with a "
+ "%select{struct|interface|union
rsmith created this revision.
rsmith added a reviewer: EricWF.
rsmith added a subscriber: cfe-commits.
rsmith set the repository for this revision to rL LLVM.
Repository:
rL LLVM
https://reviews.llvm.org/D27364
Files:
test/catch_function_03.pass.cpp
test/catch_member_function_pointer_02.pa
rsmith added inline comments.
Comment at: include/clang/AST/Decl.h:3250
+ /// This is true if this struct ends with an array marked 'flexible_array'.
+ bool HasFlexibleArrayAttr : 1;
+
How is this different from `HasFlexibleArrayMember`? Do we really need both?
rsmith added a comment.
Phabricator doesn't let me retroactively approve, but LGTM, thanks!
Repository:
rL LLVM
https://reviews.llvm.org/D27422
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo
rsmith added a comment.
Thanks!
If you look at `Sema::InstantiateClass`, we instantiate all of the attributes
there. The problem seems to be that that happens only when instantiating the
class definition, which happens after the point at which we would diagnose use
of a deprecated declaration.
rsmith added inline comments.
Comment at: include/clang/Sema/DeclSpec.h:1240
+/// in the prototype. These are generally tag types or enumerators.
+unsigned NumDeclsInPrototype : 8;
+
It seems plausible that generated code could have more than 256 such
de
rsmith added inline comments.
Comment at: lib/Lex/PPMacroExpansion.cpp:110-112
+// FIXME: shouldIgnoreMacro() in ASTWriter also stops at macros from the
+// predefines buffer in module builds. Do we need to splice to those here
+// too?
If I remember
rsmith added a comment.
Looks like we might only be making macros visible, and failing to emit the
annot_module_import token for Sema.
https://reviews.llvm.org/D26267
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi
rsmith accepted this revision.
rsmith added a comment.
This revision is now accepted and ready to land.
I'd much prefer we take this size reduction now and worry about the minor loss
of ast dump fidelity later. Please delete the commented out code though :)
https://reviews.llvm.org/D27279
__
rsmith accepted this revision.
rsmith added a comment.
This revision is now accepted and ready to land.
Thanks!
https://reviews.llvm.org/D25216
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cf
rsmith added inline comments.
Comment at: lib/Driver/Tools.cpp:284
+ // propagate -fdiagnostics-color.
+ if (StringRef(TC.GetLinkerPath()).endswith("ld.lld") &&
+ D.getDiags().getShowColors())
I don't think this will work for `-fuse-ld=$BINDIR/lld` and the
rsmith added inline comments.
Comment at: lib/Driver/Tools.cpp:284
+ // propagate -fdiagnostics-color.
+ if (StringRef(TC.GetLinkerPath()).endswith("ld.lld") &&
+ D.getDiags().getShowColors())
ruiu wrote:
> rsmith wrote:
> > I don't think this will work fo
rsmith added a comment.
Please add a test to make sure this does not fire on C++17 decomposition
declarations:
void f() {
struct X { int a, b, c; };
auto [a, b, c] = X();
}
https://reviews.llvm.org/D27621
___
cfe-commits mailing list
c
rsmith added a comment.
Generally this seems reasonable to me. I don't see any particular need to split
this patch up into smaller pieces.
Comment at: include/clang/Serialization/ASTBitCodes.h:256
EXTENSION_BLOCK_ID,
+ DIAGNOSTIC_OPTIONS_BLOCK_ID
};
--
rsmith added a comment.
I don't like calling this a "TBAA sanitizer". What we're sanitizing is the
object model and effective type rules; it seems irrelevant which specific
compiler analysis passes would result in your program misbehaving if you break
the rules. I would also expect that we will
rsmith added a comment.
> ! In https://reviews.llvm.org/D32199#731252, @hfinkel wrote:
>
>> How about renaming this to something more like `-fsanitize=type`?
>
> I'm fine with that. Do you like TypeSanitizer or TypeAccessSantizer or
> TypeAliasingSanitizer best?
I think calling it a type alias
rsmith added inline comments.
Comment at: include/clang/Sema/Sema.h:1464
+ /// Determine if \p D abd \p Suggested have a structurally compatibale
+ /// layout as described in C11 6.2.7/1.
Typo 'abd'
Comment at: lib/Parse/ParseDecl.cpp:4236-
rsmith added inline comments.
Comment at: lib/Sema/SemaOverload.cpp:3850-3853
// In Microsoft mode, prefer an integral conversion to a
// floating-to-integral conversion if the integral conversion
// is between types of the same size.
// For example:
rsmith added a comment.
> As I've currently implemented it, both reads and writes set the type of
> previously-unknown storage, and after that it says fixed (unless you memcpy
> to it, memset it, or its lifetime ends (the type gets reset on
> lifetime.start/end and for malloc/allocas/etc.). The
rsmith added a comment.
In https://reviews.llvm.org/D32199#732189, @hfinkel wrote:
> In https://reviews.llvm.org/D32199#731472, @rsmith wrote:
>
> > 1. C's "effective type" rule allows writes to set the type pretty much
> > unconditionally, unless the storage is for a variable with a declared ty
rsmith accepted this revision.
rsmith added a comment.
This revision is now accepted and ready to land.
I'm not thrilled about adding yet more predefined macros, but it really doesn't
make sense for libc++ to depend on `__GCC_*` macros when targeting Windows, nor
for these to be Windows-only, so
rsmith requested changes to this revision.
rsmith added a comment.
This revision now requires changes to proceed.
This is sadly not a correct change. The relevant requirements (C11 7.20.3/2) on
these macros are:
> Each instance of these macros shall be replaced by a constant expression
> suitab
rsmith added a comment.
Please add some test coverage for these triples.
Repository:
rL LLVM
https://reviews.llvm.org/D32269
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
rsmith added a comment.
This needs testcases (the one from your summary plus the ones in my comments
above would be good).
Comment at: lib/AST/ExprConstant.cpp:2622
// Next subobject is an array element.
- const ConstantArrayType *CAT = Info.Ctx.getAsConstantArrayT
rsmith added inline comments.
Comment at: test/std/numerics/numeric.ops/numeric.ops.gcd/gcd.pass.cpp:46
+std::gcd(static_cast(0), static_cast(0)))>::value, "");
+const bool result = static_cast>(out) ==
+std::gcd(static_cast(in1), static_cast(in2));
--
rsmith added a comment.
In https://reviews.llvm.org/D31856#733845, @efriedma wrote:
> We normally use stdint.h from the host C library, rather than our own
> version; this file is only relevant in -ffreestanding mode. So it should be
> safe to change.
Agreed; r89237 (and nearby changes: r892
rsmith added inline comments.
Comment at: lib/CodeGen/CGExprScalar.cpp:3066-3067
+} else { // Unsigned integers and pointers.
+ if (CGF.CGM.getCodeGenOpts().StrictVTablePointers &&
+ CGF.CGM.getCodeGenOpts().OptimizationLevel > 0) {
+// Based on comparis
rsmith accepted this revision.
rsmith added a comment.
This revision is now accepted and ready to land.
LGTM. I'd like to make sure we try to use something better than the first
import location for a module (that location is especially meaningless under
`-fmodules-local-submodule-visibility`), b
rsmith added inline comments.
Comment at: lib/Parse/ParseTemplate.cpp:1203-1204
+ {
+EnterExpressionEvaluationContext EnterConstantEvaluated(
+Actions, Sema::ExpressionEvaluationContext::ConstantEvaluated);
+if (isCXXTypeId(TypeIdAsTemplateArgument)) {
--
rsmith added a comment.
The change in direction from diagnosing the lvalue-to-rvalue conversion to
diagnosing the pointer arithmetic seems fine to me (and is likely a better
approach overall), but this means we should now treat a designator referring to
element 0 of an array of unknown / runtim
rsmith added a comment.
This change looks like it introduces a regression itself: given
template struct A {};
template using B = A;
B bi;
... requesting the template arguments for the type `B` changes from
producing `int` to producing `int*` with this patch, which seems to directly
oppo
rsmith accepted this revision.
rsmith added a comment.
This revision is now accepted and ready to land.
LGTM
https://reviews.llvm.org/D32455
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-c
rsmith accepted this revision.
rsmith added a comment.
This revision is now accepted and ready to land.
If we're now catching integer overflow in more cases, please add some relevant
testcases. If this is a pure refactoring that enables those additional
diagnostics to be produced in future, then
rsmith added inline comments.
Comment at: lib/AST/ExprConstant.cpp:4469-4470
{ return StmtVisitorTy::Visit(E->getSubExpr()); }
+ bool VisitObjCBoxedExpr(const ObjCBoxedExpr *E)
+{ return StmtVisitorTy::Visit(E->getSubExpr()); }
bool VisitChooseExpr(const ChooseExpr *
rsmith accepted this revision.
rsmith added a comment.
This revision is now accepted and ready to land.
LGTM
https://reviews.llvm.org/D32405
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-c
rsmith added a comment.
I'm OK with this from a mechanical perspective. But there's also a libclang
design question here: what should the libclang methods to query template
arguments for a type cursor representing an alias template specialization
actually do? Should there be some way for a libc
rsmith accepted this revision.
rsmith added a comment.
This revision is now accepted and ready to land.
LGTM
Comment at: test/Preprocessor/macro_paste_commaext.c:4
+// In the following tests, note that the output is sensitive to the
+// whitespace *preceeding* the varargs argum
rsmith accepted this revision.
rsmith added a comment.
This revision is now accepted and ready to land.
LGTM, thanks!
https://reviews.llvm.org/D32410
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listi
rsmith accepted this revision.
rsmith added a comment.
This revision is now accepted and ready to land.
Yes, let's first revert back to the clang 4.0 behavior, then please mail
cfe-dev to discuss what the right behavior should be.
Repository:
rL LLVM
https://reviews.llvm.org/D32566
__
rsmith added a comment.
Thanks, this looks good, just a couple of minor things and then it should be
ready to land.
Do you have commit access or will you need someone else to commit this for you?
Comment at: lib/AST/ExprConstant.cpp:152
+ uint64_t &A
rsmith accepted this revision.
rsmith added a comment.
This revision is now accepted and ready to land.
Thanks. Could you also add something like:
struct A {};
struct B : virtual A {};
constexpr A &a = (A&)*(B*)0;
to test/SemaCXX/constant-expression-cxx11.cpp to ensure we produce a suitabl
rsmith accepted this revision.
rsmith added a comment.
This revision is now accepted and ready to land.
Committed as r301822.
Comment at: lib/AST/ExprConstant.cpp:1301
void addUnsizedArray(EvalInfo &Info, QualType ElemTy) {
- assert(Designator.Entries.empty() && getTy
rsmith added inline comments.
Comment at: test/CXX/drs/dr4xx.cpp:1205
-namespace dr496 { // dr496: no
+namespace dr496 { // dr496: reverted by dr2095 in 5.0
struct A { int n; };
Write this as "dr496: sup 2094" and then rerun the `make_cxx_dr_status` script
rsmith accepted this revision.
rsmith added a comment.
This revision is now accepted and ready to land.
Let's go ahead with this. I've been unable to find a testcase that triggers the
problem, but we shouldn't keep a known latent bug around just because we don't
know how to expose it yet.
http
rsmith accepted this revision.
rsmith added a comment.
This revision is now accepted and ready to land.
This makes a lot of sense to me. See also r302463: I think we probably want
three levels of shadowing here: the main input shadows -fmodule-map-file, which
shadows module maps loaded implicitl
rsmith added inline comments.
Comment at: lib/Serialization/ASTReaderDecl.cpp:213-215
+ assert(isa(D) ||
+ isa(D) &&
+ "Decl doesn't have specializations.");
Can this ever fail at runtime? I'd expect the below code to not compile if `
rsmith added inline comments.
Comment at: lib/Sema/SemaLookup.cpp:4971-4972
assert(Owner && "definition of hidden declaration is not in a module");
+ assert((!isVisible(Decl) || VisibleModules.isVisible(Owner)) &&
+ "missing import for non-hidden decl?");
--
rsmith added inline comments.
Comment at: include/clang/Sema/Sema.h:1464
+ /// Determine if \p D abd \p Suggested have a structurally compatibale
+ /// layout as described in C11 6.2.7/1.
rsmith wrote:
> Typo 'abd'
Typo 'compatibale' =)
Com
rsmith accepted this revision.
rsmith added a comment.
This revision is now accepted and ready to land.
Please first check in a change that just regenerates cxx_dr_status without your
changes, to separate out the changes due to the new issues list from the
changes due to this patch. (You can go
rsmith added a comment.
Thank you, some of these test typos are ... alarming. =)
A couple of the test updates don't look quite right, but this mostly looks
great.
Comment at: test/Driver/amdgpu-features.c:1
-// RUN: %clang -### -target amdgcn -x cl -S -emit-llvm -mcpu=kaveri
rsmith added a comment.
Er, please ignore the inline review comments; those predated the realisation
that this doesn't actually fix the glibc build problem.
https://reviews.llvm.org/D16171
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
htt
rsmith requested changes to this revision.
rsmith added a comment.
This revision now requires changes to proceed.
In https://reviews.llvm.org/D16171#540261, @phabricss wrote:
> ../include/sys/stat.h:16:15: error: cannot apply asm label to function
> after its first use
> hidden_proto (__fxst
rsmith accepted this revision.
rsmith added a comment.
This revision is now accepted and ready to land.
LGTM
https://reviews.llvm.org/D33013
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-c
rsmith added a comment.
I'd like to suggest an alternative design: don't add a new attribute., and
instead change the semantics of `__attribute__((overloadable))` to permit at
most one non-overloadable function in an overload set. That one function would
implicitly get the `transparently_overlo
rsmith added a comment.
Clang's regression test suite is not a sensible home for these tests. We should
have a home and a plan for system-specific integration tests, but this is not
it. Perhaps this should instead be a part of LNT / the test-suite project?
https://reviews.llvm.org/D32178
__
rsmith added inline comments.
Comment at: test/SemaCXX/warn-shadow.cpp:214
+void handleLinkageSpec() {
+ typedef void externC; // expected-warning {{declaration shadows a typedef in
linkage specification}}
+}
We should be producing a diagnostic talking about th
rsmith added inline comments.
Comment at: lib/Parse/ParseExpr.cpp:203-222
+ // Create the ExpressionEvaluationContext on the stack - but only if asked
to.
+ struct EnterExpressionEvaluationContextConditionalRAII {
+llvm::AlignedCharArray
+MyStackStorage;
+const
rsmith accepted this revision.
rsmith added a comment.
This revision is now accepted and ready to land.
LGTM, thanks!
https://reviews.llvm.org/D33207
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listi
rsmith accepted this revision.
rsmith added a comment.
Some comments, but I'm happy for you to go ahead and commit after addressing
them. Thanks!
Comment at: include/clang/Lex/Preprocessor.h:2004
+ ArrayRef getPreambleConditionalStack() const
+ { return PreambleConditionalSt
rsmith added a comment.
Should I assume our "misplaced ellipsis" diagnostic requires that we
disambiguate the ill-formed ellipsis-after-declarator-id as a declaration in
some cases? If so, do we have tests for that somewhere?
Comment at: include/clang/Parse/Parser.h:2138
+ T
rsmith added inline comments.
Comment at: include/clang/Parse/Parser.h:1956-1957
AccessSpecifier AS, DeclSpecContext DSC);
- void ParseEnumBody(SourceLocation StartLoc, Decl *TagDecl);
+ void ParseEnumBody(SourceLocation StartLoc, Decl *TagDecl,
+
rsmith added a comment.
In https://reviews.llvm.org/D9#759146, @hubert.reinterpretcast wrote:
> The `check-all` target passes even if the ellipsis-after-declarator-id
> disambiguation as a declarator is removed entirely.
[...]
> So, on the whole, the stray ellipsis treatment is both too c
rsmith added a comment.
This is supposed to be handled by `Sema::DeclareGlobalAllocationFunction`:
DeclContext::lookup_result R = GlobalCtx->lookup(Name);
for (DeclContext::lookup_iterator Alloc = R.begin(), AllocEnd = R.end();
Alloc != AllocEnd; ++Alloc) {
// Only look at non-temp
rsmith accepted this revision.
rsmith added a comment.
This revision is now accepted and ready to land.
LGTM, thanks!
https://reviews.llvm.org/D9
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listi
rsmith accepted this revision.
rsmith added a comment.
This revision is now accepted and ready to land.
LGTM
Comment at: lib/Sema/AnalysisBasedWarnings.cpp:338
+ // In a coroutine, only co_return statements count as normal returns.
Remember
+ // if we are processing the coro
rsmith added inline comments.
Comment at: lib/CodeGen/CGCoroutine.cpp:273
+
+// FIXME: There must be a cleaner way to do this!
+if (auto *Cleanup =
dyn_cast_or_null(&*CGF.EHStack.begin())) {
It doesn't seem safe to assume that a prior `EHCleanupScope` wo
rsmith added inline comments.
Comment at: include/clang/AST/StmtCXX.h:128
+struct CXXTryStmt::OnStack : CXXTryStmt {
+ alignas(CXXTryStmt) Stmt *Stmts[2];
+ OnStack(SourceLocation tryLoc, Stmt *tryBlock, Stmt *handler)
This makes more assumptions about record l
rsmith accepted this revision.
rsmith added a comment.
This revision is now accepted and ready to land.
LGTM
https://reviews.llvm.org/D31608
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-c
rsmith accepted this revision.
rsmith added a comment.
This revision is now accepted and ready to land.
Does it still make sense for us to have a `UO_Coawait` at all? As I recall, the
only purpose it ever had was to represent a dependent `co_await` expression
that couldn't yet be resolved to a `
rsmith added a comment.
Do we really need two different notions of "definition" and "odr definition"
here? What goes wrong if we always treat the "instantiation of a friend
function definition" case as being a definition?
Comment at: include/clang/AST/Decl.h:1789
+ // fu
rsmith added inline comments.
Comment at: lib/AST/ItaniumMangle.cpp:2329-2333
+ // __unaligned is not currently mangled in any way. This implies that it is
+ // not a relevant qualifier for substitutions (while CVR and maybe others
+ // are). This triggers an assertion when th
rsmith accepted this revision.
rsmith added a comment.
This revision is now accepted and ready to land.
LGTM
https://reviews.llvm.org/D31692
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-c
rsmith added a comment.
Counterproposal: in `-std=*++14` onwards, treat this construct as a
user-defined literal, but fall back on the built-in interpretation if name
lookup doesn't find an `operator""i` function. (The two interpretations only
conflict if the source code explicitly does somethi
rsmith added inline comments.
Comment at: include/clang/Serialization/ASTReader.h:551
+ llvm::SmallVector PendingLazySpecializationIDs;
+
I'm not sure we can safely use global state for this: loading update records
can trigger the import of a declaration and
rsmith added inline comments.
Comment at: lib/Sema/SemaExprCXX.cpp:2661-2669
+// If we're in local visibility mode, we reuse this allocation function
+// in all submodules of the current module. To make sure that the other
+// submodules can lookup this function, we u
rsmith accepted this revision.
rsmith added a comment.
This revision is now accepted and ready to land.
I like it, and it seems like it would nicely generalize if there are more cases
that we find need this treatment.
Comment at: clang/lib/Lex/LiteralSupport.cpp:676-682
-
rsmith added a comment.
In https://reviews.llvm.org/D29951#750885, @v.g.vassilev wrote:
> In order to create a reasonable test I need to use
> `-error-on-deserialized-decl` and I hit a bug:
> https://bugs.llvm.org/show_bug.cgi?id=32988
>
> Richard, could you help me out with the test? Maybe we
101 - 188 of 188 matches
Mail list logo
