compnerd added inline comments.
Comment at: lib/CodeGen/CGObjCMac.cpp:3961
+ // linkage so that the linker preserves the symbol name.
+ llvm::GlobalValue::LinkageTypes LT =
+ Section.empty() || Section.startswith("__DATA")
Hmm, when would you have a metada
compnerd added a comment.
@rupprecht - actually, I *really* like the idea of putting the experimental in
the name. Also agree with you on the `ifo` vs `ifso`. Although, if we go with
`ifo`, I would prefer to bikeshed the option to `-emit-interface` (well with
the experimental stuck in as well
compnerd added inline comments.
Comment at: clang/include/clang/Driver/Options.td:626
HelpText<"Use the LLVM representation for assembler and object files">;
+def emit_ifso : Flag<["-"], "emit-interface-stubs">, Flags<[CC1Option]>,
Group,
+ HelpText<"Generate Inteface Stub F
compnerd added a comment.
@steven_wu - yeah, `-mred-zone`, `-mno-red-zone` does impact the ABI, at least
on x86_64. It changes the way that the arguments are spilled and the stack
layout.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D61627/new/
compnerd added inline comments.
Comment at: clang/include/clang/Driver/Options.td:626
HelpText<"Use the LLVM representation for assembler and object files">;
+def emit_ifso : Flag<["-"], "emit-interface-stubs">, Flags<[CC1Option]>,
Group,
+ HelpText<"Generate Inteface Stub F
compnerd added a comment.
Well, Apple's ARM64 ABI also has red zones and they are similar to x64 in the
sense that they can be used for spilling locals. I believe that the ILP32
variant retains the red zones as well, so, yes, this would impact the ABI
unfortunately, and the platform guarantees
compnerd added a comment.
I don't really have a problem with this change. But, if we make this change,
please include a change to the release notes *now*. This is something which
may catch users off guard and confuse them and require them to go looking for
what happened.
As an aside, I think
compnerd accepted this revision.
compnerd added a comment.
This revision is now accepted and ready to land.
(Accepting with the condition that you will update the release notes before
committing)
Repository:
rC Clang
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D61689/new/
https://r
compnerd added inline comments.
Herald added a subscriber: libcxx-commits.
Comment at: include/filesystem:1396
- _LIBCPP_FUNC_VIS
void __create_what(int __num_paths);
This possibly changes the meaning on other targets. What was the error that
this trigger
compnerd created this revision.
compnerd added a reviewer: steven_wu.
When embedding bitcode, only a subset of the arguments should be recorded into
the bitcode compilation command line. The frontend job is split into two jobs,
one which will generate the bitcode. Ensure that the arguments for t
compnerd added inline comments.
Comment at: test/Driver/Xarch.c:5
+// RUN: not grep ' "-O3" ' %t.log
+// RUN: grep "argument unused during compilation: '-Xarch_i386 -O3'" %t.log
// RUN: not %clang -target i386-apple-darwin9 -m32 -Xarch_i386 -o -Xarch_i386
-S %s -S -Xarch_i386 -
compnerd accepted this revision.
compnerd added a comment.
This revision is now accepted and ready to land.
This looks fine to me, but this definitely should have an accompanying test.
As to the other items you mention, the current section naming actually is
important as it enables the CFString
compnerd closed this revision.
compnerd added a comment.
SVN r342929 (with tests)
Repository:
rC Clang
https://reviews.llvm.org/D52252
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-comm
compnerd requested changes to this revision.
compnerd added inline comments.
This revision now requires changes to proceed.
Comment at: lib/Basic/Targets/AMDGPU.cpp:263
MaxAtomicPromoteWidth = MaxAtomicInlineWidth = 64;
+#if _WIN32
+ WCharType = UnsignedShort;
---
compnerd accepted this revision.
compnerd added a comment.
I don't think that there is a guarantee that compiler-rt and clang are upgraded
in lockstep. At least for the builtins, there is a relatively stable
interface. However, I don't think that at this point the Windows MSVC
environment dep
compnerd added inline comments.
Comment at: lib/CodeGen/ItaniumCXXABI.cpp:2990-2993
+ bool IsMinGW =
+ CGM.getContext().getTargetInfo().getTriple().getEnvironment() ==
+ llvm::Triple::GNU &&
+ CGM.getContext().getTargetInfo().getTriple().isOSWi
compnerd added a comment.
I think that splitting this up in a series of patches would be much better.
The first patch should be to do the entirely mechanical change of the
visibility attribute. It is a separate library and needs its own visibility
attribute. That would significantly slim dow
compnerd accepted this revision.
compnerd added inline comments.
This revision is now accepted and ready to land.
Comment at: include/experimental/__config:57
+#if defined(_LIBCPP_OBJECT_FORMAT_COFF)
+# define _LIBCPPX_TYPE_VIS
hamzasood wrote:
> smeenai wrote:
compnerd requested changes to this revision.
compnerd added a comment.
This revision now requires changes to proceed.
I think we should avoid this logic entirely and use CMake to do this.
https://reviews.llvm.org/D37133
___
cfe-commits mailing list
compnerd requested changes to this revision.
compnerd added a comment.
This revision now requires changes to proceed.
If we handle this in CMake, this will be unnecessary.
https://reviews.llvm.org/D37134
___
cfe-commits mailing list
cfe-commits@list
compnerd added a comment.
`$` should give you the `.o` or `.obj`
files used to construct the library.
https://reviews.llvm.org/D37133
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
compnerd accepted this revision.
compnerd added a comment.
This revision is now accepted and ready to land.
ick
https://reviews.llvm.org/D37468
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cf
compnerd added a comment.
The change looks good. Can you please add some test cases for this? Or do
existing test cases cover this already?
https://reviews.llvm.org/D37496
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.
compnerd added inline comments.
Comment at: bindings/python/tests/cindex/test_tls_kind.py:14
+int tls_none;
+thread_local tls_dynamic;
+""", lang = 'cpp')
Can we add a test case for static TLS as well please? Also, I think that we
should add a test case for `__
compnerd requested changes to this revision.
compnerd added inline comments.
This revision now requires changes to proceed.
Comment at: lib/CMakeLists.txt:189
+macro(add_msvcrt_defs_if_needed target)
+ if(WIN32 AND NOT MINGW)
Please use `function` rather than
compnerd accepted this revision.
compnerd added inline comments.
This revision is now accepted and ready to land.
Comment at: bindings/python/tests/cindex/test_tls_kind.py:32
+__declspec(thread) int tls_declspec;
+""", lang = 'cpp', flags=['-fms-extensions', '-target', 'amd64-win
compnerd added a comment.
Do you have commit rights or would you like me to commit this on your behalf?
https://reviews.llvm.org/D37577
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commit
compnerd accepted this revision.
compnerd added a comment.
This revision is now accepted and ready to land.
Why the additional inclusion of the support in the files which are not modified
to use functions? (e.g. `src/stdexcept.cpp`). LG otherwise.
Comment at: src/include/ato
compnerd closed this revision.
compnerd added a comment.
SVN r313111
https://reviews.llvm.org/D37577
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
compnerd added a comment.
I'm happy to pick this up again. LMK what needs to be addressed. AFAIK, no
constructors will be invoked as the class is just a wrapper over static data so
we don't need the constructors. I would rather do the undecorating stuff in a
follow-up.
https://reviews.llvm
compnerd commandeered this revision.
compnerd edited reviewers, added: smeenai; removed: compnerd.
compnerd added inline comments.
Comment at: include/typeinfo:100
+
+ static const char *__name(const struct type_info::__data *__data);
+ static size_t __hash(const struct type_in
compnerd updated this revision to Diff 115326.
https://reviews.llvm.org/D28212
Files:
include/typeinfo
src/support/runtime/exception_msvc.ipp
src/typeinfo.cpp
Index: src/typeinfo.cpp
===
--- src/typeinfo.cpp
+++ src/typeinfo.c
compnerd marked 4 inline comments as done.
compnerd added a comment.
Also added `_NOEXCEPT` to `__compare`.
https://reviews.llvm.org/D28212
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-co
compnerd closed this revision.
compnerd added a comment.
SVN r313344
https://reviews.llvm.org/D28212
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
compnerd created this revision.
compnerd added a project: clang.
Herald added subscribers: fedor.sergeev, javed.absar.
Move the logic for determining the `wchar_t` type information into the
driver. Rather than passing the single bit of information of
`-fshort-wchar` indicate to the frontend the d
compnerd added a comment.
We could leave the defaults there as they stand, and only have the new flags
alter the default. However, it seems that just paying the cost of adjusting
the tests once isn't too bad. To me, it seems that having one instance of the
horrible logic for determining the t
compnerd added inline comments.
Comment at: lib/Driver/ToolChains/Clang.cpp:2659
+
+ const bool IsAPCSABI =
+ IsARM && (IsGNUEnvironment || IsNetBSD ||
rnk wrote:
> This target detection logic is insanely complicated, and I'm not convinced
> it's simpler t
compnerd updated this revision to Diff 115510.
compnerd added a comment.
Try to clarify the logic for APCS ABI.
Repository:
rL LLVM
https://reviews.llvm.org/D37891
Files:
include/clang/Basic/DiagnosticFrontendKinds.td
include/clang/Basic/LangOptions.def
include/clang/Driver/CC1Options.
compnerd added inline comments.
Comment at: lib/CodeGen/CodeGenModule.cpp:477
Context.getTypeSizeInChars(Context.getWideCharType()).getQuantity();
- assert((LangOpts.ShortWChar ||
- llvm::TargetLibraryInfoImpl::getTargetWCharSize(Target.getTriple())
==
-
compnerd added inline comments.
Comment at: lib/CodeGen/CodeGenModule.cpp:477
Context.getTypeSizeInChars(Context.getWideCharType()).getQuantity();
- assert((LangOpts.ShortWChar ||
- llvm::TargetLibraryInfoImpl::getTargetWCharSize(Target.getTriple())
==
-
compnerd added a comment.
Sure, Ill get this merged shortly.
https://reviews.llvm.org/D37573
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
compnerd closed this revision.
compnerd added a comment.
SVN r314009
https://reviews.llvm.org/D37573
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
compnerd added inline comments.
Comment at: clang/lib/Frontend/CompilerInvocation.cpp:1690
+ std::make_pair(frontend::GenerateInterfaceTBEExpV1, false));
+ if (!ProgramActionPair.second)
+Diags.Report(diag::err_drv_invalid_value)
I t
compnerd accepted this revision.
compnerd added inline comments.
This revision is now accepted and ready to land.
Comment at: clang/lib/Frontend/InterfaceStubFunctionsConsumer.cpp:91
+ std::vector MangledNames = CGNameGen.getAllManglings(ND);
+ if (isa(ND) || isa(ND)) {
compnerd added a comment.
Could you please add a test to ensure that Darwin defaults to the old behaviour?
Comment at: include/clang/Basic/LangOptions.h:142
+/// Attempt to be ABI-compatible with code generated by Clang 8.0.x
+/// (https://reviews.llvm.org/D59744). This
compnerd added inline comments.
Comment at: clang/include/clang/AST/Mangle.h:19
+#include "clang/AST/DeclCXX.h"
+#include "clang/AST/DeclObjC.h"
#include "clang/AST/Type.h"
Move these into the implementation, forward declarations should be sufficient.
compnerd added inline comments.
Comment at: clang/include/clang/AST/Mangle.h:248
+
+struct ASTNameGenerator {
+ std::unique_ptr MC;
Please make this a `class` and hide the members, exposing just the constructor
and the 3 methods as @aaron.ballman pointed out th
compnerd accepted this revision.
compnerd added inline comments.
This revision is now accepted and ready to land.
Comment at: clang/lib/AST/Mangle.cpp:343
-std::vector
-ASTNameGenerator::getAllManglings(const ObjCContainerDecl *OCD) {
+ std::vector getAllManglings(const ObjCCo
compnerd accepted this revision.
compnerd added a comment.
This revision is now accepted and ready to land.
Yeah, that makes sense, the common path uses `{{.*}}` as the value itself is
uninteresting.
Repository:
rC Clang
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D64772/new/
https
compnerd added a comment.
This looks good to me generally. I don't fully understand the reason for `u`
being kept, is that something you intend to clean up in a subsequent patch?
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D65176/new/
https://re
compnerd added inline comments.
Comment at: clang/include/clang/Driver/Types.def:39-45
+// Some of the options in Flags have been removed, so far those are:
+// a - The type should only be assembled: Now, check that Phases contains
+// phases::Assemble but not phases::Compi
compnerd closed this revision.
compnerd added a comment.
SVN r367042
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D64656/new/
https://reviews.llvm.org/D64656
___
cfe-commits mailing list
cfe-commits@li
compnerd added inline comments.
Comment at: clang/lib/Driver/Types.cpp:113
+ Id == TY_HIP_FATBIN);
+// clang-format on
}
I think this is better written as:
```
static const clang::driver::types::ID kStaticLangageTypes[] = {
TY_CUDA_DEVICE, TY_HIP_DE
compnerd added a comment.
Rather than silently ignoring tests when the DirectoryWatcher isn't created,
can you please print an error message and exit with an error code to indicate
the test failed?
Comment at: clang/lib/DirectoryWatcher/linux/DirectoryWatcher-linux.cpp:331
+
compnerd added a comment.
BTW, I think that we should add a test case to ensure that we see the error in
the case that the inotify fds are exhausted. We should be able to create a
process and set the limit for that process to 0/1 and use that to trigger the
failure.
Repository:
rG LLVM Git
compnerd added a comment.
`-ldl` doesn't work on all platforms (e.g. android, FreeBSD, etc). `-lpthread`
is wrong - if you want to add that, I think that we need to improve the
`-thread-model` flag in clang first (it currently just always passes `posix`,
which is ignored; but would identify th
compnerd added inline comments.
Comment at: clang/lib/DirectoryWatcher/mac/DirectoryWatcher-mac.cpp:218
+return llvm::make_error(
+std::string("Path.empty() error: ") + strerror(errno),
+llvm::inconvertibleErrorCode());
compnerd wrote:
> Simil
compnerd added a comment.
@gribozavr I think that this usage here is actually useful because it
a) tests the actual behaviour
b) provides example code for other users
The check here ensures that the rest of the code is properly executed, *but*
because the error is not actually consumed (you need
compnerd added a comment.
@lhames - I like the `logAllUnhandledErrors`!
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D65829/new/
https://reviews.llvm.org/D65829
___
cfe-commits mailing list
cfe-commits
compnerd added inline comments.
Comment at: clang/include/clang/Driver/Options.td:333
HelpText<"Print (but do not run) the commands to run for this
compilation">;
+def _HASH_HASH_HASH_VERBATIM : Flag<["-"], "###-verbatim">,
+Flags<[DriverOption, CoreOption]>,
--
compnerd added inline comments.
Comment at: clang/lib/Basic/Targets/ARM.cpp:904
+ case 'j': // An immediate integer between 0 and 65535 (valid for MOVW)
+if (CPUAttr.equals("6T2") ||
+ArchVersion >= 7) // only available in ARMv6T2 and above
I would h
compnerd accepted this revision.
compnerd added a comment.
This revision is now accepted and ready to land.
Seems like it should be equivalent.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D65969/new/
https://reviews.llvm.org/D65969
compnerd added inline comments.
Comment at: clang/include/clang/Driver/Types.h:107
+llvm::opt::DerivedArgList &DAL, ID Id,
+llvm::SmallVectorImpl &Phases);
This really makes things confusing, perhaps renam
compnerd added a comment.
Tests?
Comment at: clang/lib/Driver/Driver.cpp:3420
+ // Add a link action if necessary.
+ if (!MergerInputs.empty()) {
This isn't really a link action ...
Comment at: clang/lib/Driver/Driver.cpp:3423
+Action
compnerd added a comment.
Ugh, this is still not the most structured handling of the arguments. But,
yeah, this seems like it should be equivalent. Fine by me if @aaron.ballman
has no more comments.
Repository:
rC Clang
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D66058/new/
htt
compnerd created this revision.
compnerd added reviewers: craig.topper, srhines.
Herald added a subscriber: jfb.
Herald added a project: clang.
The android target assumes that for the x86_64 target, the CPU supports SSE4.2
and `popcnt`. This implies that the CPU is Nehalem or newer. This should
compnerd added a comment.
@lebedev.ri - sure, I will add a driver test to ensure that the feature is set
on the command line when invoked from the driver, however, I don't think that
there is really much in terms of testing that you can do for this type of stuff
other than throw a large corpus
compnerd updated this revision to Diff 206508.
compnerd added a comment.
add additional context and test case
Repository:
rC Clang
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D63774/new/
https://reviews.llvm.org/D63774
Files:
lib/Driver/ToolChains/Arch/X86.cpp
test/Driver/andro
compnerd updated this revision to Diff 206519.
compnerd added a comment.
Move test case around
Repository:
rC Clang
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D63774/new/
https://reviews.llvm.org/D63774
Files:
lib/Driver/ToolChains/Arch/X86.cpp
test/Driver/clang-translation.c
compnerd added a comment.
@craig.topper, hmm, what happens in terms of CG when LAHF/SAHF are not
available? I assume its just worse CG as you could spill AH onto the stack and
do a load/store. This actually results in library calls which may not be
possible to fulfill.
Repository:
rC Clan
compnerd closed this revision.
compnerd added a comment.
SVN r364352
Repository:
rC Clang
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D63774/new/
https://reviews.llvm.org/D63774
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
ht
compnerd added a comment.
The explicit list I think is way better for readability, this is a nice
starting point for cleaning this up.
Comment at: clang/include/clang/Driver/Types.def:18
// TYPE(NAME, ID, PP_TYPE, TEMP_SUFFIX, FLAGS)
Please update the com
compnerd added inline comments.
Comment at: clang/lib/Driver/Types.cpp:29
+ { NAME, FLAGS, TEMP_SUFFIX, TY_##PP_TYPE, PHASES, },
+#define PHASES llvm::SmallVector
#include "clang/Driver/Types.def"
I think that we can abuse the preprocessor a bit and get somethi
compnerd added inline comments.
Comment at: clang/include/clang/Basic/Attr.td:1948
+def RequireDesignatedInit : InheritableAttr {
+ let Spellings = [GNU<"require_designated_init">];
+ let Subjects = SubjectList<[Type]>;
This seems like an extension? I think th
compnerd created this revision.
compnerd added reviewers: smeenai, beanz, EricWF.
Herald added subscribers: libcxx-commits, ldionne, mgorny.
Rather than building up a list to iterate over later, just create multiple
`install` commands based on the configuration. This makes it easier to see
what
compnerd marked an inline comment as done.
compnerd added a comment.
@ldionne - that was exactly the motivation for this change - it always takes me
a couple of reads to figure out what we are trying to do here.
Comment at: src/CMakeLists.txt:441
+ if (LIBCXX_INSTALL_STATIC_L
compnerd closed this revision.
compnerd added a comment.
SVN r365562
Repository:
rCXX libc++
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D64383/new/
https://reviews.llvm.org/D64383
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
compnerd added a comment.
I don't see any cases where `[[clang::required]]` is tested, am I missing
something?
Comment at: clang/test/SemaCXX/attr-designated-init-required.cpp:3
+
+#define ATTR [[clang::designated_init_required]]
+
Why the macro?
===
compnerd added inline comments.
Comment at: clang/include/clang/Basic/Attr.td:1948
+def RequiresDesignator : InheritableAttr {
+ let Spellings = [Clang<"requires_designator">];
+ let Subjects = SubjectList<[Record]>;
aaron.ballman wrote:
> Hmm, after making thi
compnerd added a comment.
This really needs more tests. Please add a positive/negative test for the
driver argument. Please try to organise the tests a bit to show what it is
that they are testing, emission of public functions, not of protected functions
or hidden functions. Behaviour with C
compnerd added a comment.
How about some cases for:
- global variable which is `static` and `extern`'ed
- global variable which is `static` defined in a function which is `static`
- global variable which is `static` defined in a function which is *not*
`inline`
- global variable which is `static
compnerd added inline comments.
Comment at: lib/Driver/ToolChains/Clang.cpp:3675
+// Render target options such as -fuse-init-array on modern ELF platforms.
+TC.addClangTargetOptions(Args, CmdArgs, JA.getOffloadingDeviceKind());
+
Hmm, what other argument
compnerd accepted this revision.
compnerd added a comment.
This revision is now accepted and ready to land.
Sounds safe enough to me. I think that it would be nice to add a test that
checks the hexagon flags as well.
Repository:
rC Clang
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/
compnerd accepted this revision.
compnerd added a comment.
This revision is now accepted and ready to land.
Please do clang-format this. If this is already in the wild, then, I suppose
that we don't have the option, but, this seems like something that should be
written by the author of the asse
compnerd added inline comments.
Comment at: CodeGen/CodeGenModule.cpp:2957-2958
!getCodeGenOpts().LTOVisibilityPublicStd &&
- !getTriple().isWindowsGNUEnvironment()) {
+ !getTriple().isWindowsGNUEnvironment() &&
+ !getTriple().isWindowsMSVCEn
compnerd closed this revision.
compnerd added a comment.
SVN r348939
Repository:
rC Clang
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D55586/new/
https://reviews.llvm.org/D55586
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
ht
abdulras added a comment.
@rnk, I agree that it is a performance optimization, and it actually is in the
hot path. I can understand the motivation for the jump. I think that the
trampoline might also be a problem for lldb as it (or used to?) statically
analyzes to see if it is jumping to `obj
compnerd added inline comments.
Comment at: clang/include/clang/Driver/Phases.h:24
+Link,
+IfsMerge
};
Trailing comma please
Comment at: clang/lib/Driver/Driver.cpp:3341
llvm::SmallVector FullPL;
-types::getCompilationPhases
compnerd added inline comments.
Comment at: clang/lib/Driver/Driver.cpp:3372
+ if (Phase == phases::IfsMerge) {
+assert(Phase == PL.back() && "merging must be final compilation
step.");
+MergerInputs.push_back(Current);
Does the interface me
compnerd accepted this revision.
compnerd added inline comments.
This revision is now accepted and ready to land.
Comment at: clang/lib/Driver/Driver.cpp:3372
+ if (Phase == phases::IfsMerge) {
+assert(Phase == PL.back() && "merging must be final compilation
step.")
compnerd added a comment.
I know that the Windows SDK definitely declares the `__va_start` function. Did
you try building something like swift against the Windows SDK with this change?
https://reviews.llvm.org/D45383
___
cfe-commits mailing list
c
compnerd accepted this revision.
compnerd added a comment.
This revision is now accepted and ready to land.
Sorry for the delay, didn't see the changes earlier.
Comment at: clang/lib/CodeGen/CGAtomic.cpp:883
if (UseLibcall) {
+CGM.getDiags().Report(E->getLocStart(), diag
compnerd accepted this revision.
compnerd added a comment.
This revision is now accepted and ready to land.
I definitely like the clean up. Not sure I understand the motivation for the
`__libcpp_relaxed_store`, but I suppose thats because its a copy from libc++
where it may be more useful.
Re
compnerd added a comment.
Snipping bits from `va_defs.h`:
#elif defined _M_ARM64
void __cdecl __va_start(va_list*, ...);
#define __crt_va_start_a(ap,v) ((void)(__va_start(&ap, _ADDRESSOF(v),
_SLOTSIZEOF(v), __alignof(v), _ADDRESSOF(v
...
#elif defined _M_X64
compnerd added a comment.
I'm not sure I understand this. The proper location for libc++ on the darwin
layout is in the SDK, not relative to the driver. The default behaviour is
similar to cross-compiling, and with a (derived) SDK. This definitely needs to
be reviewed by @dexonsmith
Reposi
compnerd accepted this revision.
compnerd added a comment.
Some cleanup suggestions included, but I like the change overall.
Comment at: lib/Driver/ToolChains/CommonArgs.cpp:1250
+ // Setup statistics file output.
+ if (const Arg *A = Args.getLastArg(options::OPT_save_stats_E
compnerd added inline comments.
Comment at: clang/lib/CodeGen/CGAtomic.cpp:883
if (UseLibcall) {
+CGM.getDiags().Report(E->getLocStart(), diag::warn_atomic_op_misaligned);
+
t.p.northover wrote:
> compnerd wrote:
> > It is kinda unfortunate that you need t
compnerd added a comment.
I'm torn on this. The other `-print` options will perform the validation
implicitly at the higher level before calling the inner functions. It is
certainly reasonable to support that, but, for the common path, this check
seems unnecessary (and this function is used e
compnerd accepted this revision.
compnerd added a comment.
This revision is now accepted and ready to land.
Do you have commit access or do you need someone to commit this on your behalf?
Repository:
rC Clang
https://reviews.llvm.org/D45814
___
c
compnerd closed this revision.
compnerd added a comment.
SVN r331296
Repository:
rC Clang
https://reviews.llvm.org/D45814
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
101 - 200 of 666 matches
Mail list logo
