[PATCH] D50945: [Lex] Make HeaderMaps a unique_ptr vector

kristina added a comment. Given the context (class an file name itself) and documentation around the function, I don't think in this particular case it improves readability or maintainability, the lifetime of the `HeaderMap` is (IMHO) fairly obvious from the const qualifier and from the documen

[PATCH] D50413: [libunwind][include] Add some missing definitions to .

kristina added a comment. This doesn't seem like a great idea to me, it's a negligible amount of type safety gained over using `stdint.h` types most people are more familiar with anyway. If you need something niche like those typedefs it's probably better to include the header and define them y

[PATCH] D50564: Add support for SEH unwinding on Windows.

kristina added a comment. I'm all for this change except the core issue is that you're using libunwind as a shim around the actual unwinding API provided by Windows. It would be nice to have something that did not have to do that and was capable of performing unwinding of SEH-style exceptions w

[PATCH] D50564: Add support for SEH unwinding on Windows.

kristina added a comment. In https://reviews.llvm.org/D50564#1206393, @mstorsjo wrote: > In https://reviews.llvm.org/D50564#1206370, @cdavis5x wrote: > > > In https://reviews.llvm.org/D50564#1206302, @kristina wrote: > > > > > I'm all for this change except the core issue is that you're using >

[PATCH] D50564: Add support for SEH unwinding on Windows.

kristina added a comment. In https://reviews.llvm.org/D50564#1208169, @cdavis5x wrote: > OK, I've removed some of the dependencies on Windows-specific types. The code > in `Unwind-seh.cpp` is very Windows-specific, though, so I left it alone. Much appreciated, thank you for your work. LGTM in

[PATCH] D50564: Add support for SEH unwinding on Windows.

kristina added inline comments. Comment at: src/UnwindCursor.hpp:1801 + if (pc != getLastPC()) { +UNWIND_INFO *xdata = reinterpret_cast(base + unwindEntry->UnwindData); +if (xdata->Flags & (UNW_FLAG_EHANDLER|UNW_FLAG_UHANDLER)) { mstorsjo wrote: > I can

[PATCH] D50564: Add support for SEH unwinding on Windows.

kristina added a comment. I'd say LGTM since it's an introduction of any sort of **runtime** within the LLVM project scope that deals with SEH specifically. So far all the published code is pretty much exclusively related to Clang/LLVM IR and MC support for codegen of SEH related code, but with

[PATCH] D50413: [libunwind][include] Add some missing definitions to .

kristina added a comment. Probably not, I'm personally against adding more and more typedefs for things that are just `stdint/inttypes.h` stuff and I say here the change is insignificant enough to warrant having to define even more types. It's not causing any problems but unless there are actua

[PATCH] D51507: Allow all supportable attributes to be used with #pragma clang attribute.

kristina added a comment. LGTM. Would allow for less repetition, ie. `always_inline` is an excellent candidate for cases like small functions that act as wrappers around bits of context sensitive inline asm and are usually clustered together in a header, a lot of embedded use cases come to min

[PATCH] D51507: Allow all supportable attributes to be used with #pragma clang attribute.

kristina added inline comments. Comment at: test/Misc/pragma-attribute-supported-attributes-list.test:58 // CHECK-NEXT: IFunc (SubjectMatchRule_function) +// CHECK-NEXT: InitPriority (SubjectMatchRule_variable) // CHECK-NEXT: InternalLinkage (SubjectMatchRule_variable, Subject

[PATCH] D51507: Allow all supportable attributes to be used with #pragma clang attribute.

kristina added inline comments. Comment at: test/Misc/pragma-attribute-supported-attributes-list.test:100 +// CHECK-NEXT: ObjCReturnsInnerPointer (SubjectMatchRule_objc_method, SubjectMatchRule_objc_property) +// CHECK-NEXT: ObjCRootClass (SubjectMatchRule_objc_interface) // CH

[PATCH] D51507: Allow all supportable attributes to be used with #pragma clang attribute.

kristina added inline comments. Comment at: test/Misc/pragma-attribute-supported-attributes-list.test:100 +// CHECK-NEXT: ObjCReturnsInnerPointer (SubjectMatchRule_objc_method, SubjectMatchRule_objc_property) +// CHECK-NEXT: ObjCRootClass (SubjectMatchRule_objc_interface) // CH

[PATCH] D51507: Allow all supportable attributes to be used with #pragma clang attribute.

kristina added a comment. Giving it a second glance, just as an idea, maybe it's better to leave ObjC pragmas out for now, for another more narrower-scope revision/review specific to ObjC(/Swift)? Since they could cause incorrect code generation if combined in odd ways (as well as making no sen

[PATCH] D50246: [RISCV] Add support for computing sysroot for riscv32-unknown-elf

kristina added a comment. Going to land it as it's approved by code owner. Repository: rC Clang https://reviews.llvm.org/D50246 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D50246: [RISCV] Add support for computing sysroot for riscv32-unknown-elf

kristina closed this revision. kristina added a comment. Closed by https://reviews.llvm.org/rC341655. Repository: rC Clang https://reviews.llvm.org/D50246 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailm

[PATCH] D50246: [RISCV] Add support for computing sysroot for riscv32-unknown-elf

kristina reopened this revision. kristina added a comment. This revision is now accepted and ready to land. Seems to be causing a test failure for someone (per comment in https://reviews.llvm.org/rL341655), reopened it for now. Repository: rC Clang https://reviews.llvm.org/D50246

[PATCH] D50246: [RISCV] Add support for computing sysroot for riscv32-unknown-elf

kristina added a comment. In https://reviews.llvm.org/D50246#1229068, @rogfer01 wrote: > Thanks for reopening this @kristina. > > I suggest passing `--sysroot=` to make sure we see the expected behaviour > when the sysroot is actually empty. > > Note that this would not really test the scenario

[PATCH] D50246: [RISCV] Add support for computing sysroot for riscv32-unknown-elf

kristina added a comment. In https://reviews.llvm.org/D50246#1229152, @rogfer01 wrote: > Hi @kristina . > > Sure, I didn't mean to do that broader change here. Apologies if it read that > way. > > Would it be acceptable to add an empty `--sysroot=` to the test? I can post > the change for revie

[PATCH] D50246: [RISCV] Add support for computing sysroot for riscv32-unknown-elf

kristina closed this revision. kristina added a comment. In https://reviews.llvm.org/D50246#1229191, @lebedev.ri wrote: > In https://reviews.llvm.org/D50246#1229177, @kristina wrote: > > > In https://reviews.llvm.org/D50246#1229152, @rogfer01 wrote: > > > > > Hi @kristina . > > > > > > Sure, I di

[PATCH] D51899: Remove extraneous ".a" suffix from baremetal clang_rt.builtins when compiling for baremetal.

kristina added a comment. Please submit your diffs with context in the future (`-U9`). Also this seems like it would break a lot, `.a` is just a generic extension for an object archive file, I don't think the extension is the issue unless you're using some sort of an exotic linker. https:

[PATCH] D51899: Remove extraneous ".a" suffix from baremetal clang_rt.builtins when compiling for baremetal.

kristina added a comment. The library shouldn't be an object file (it could be if you changed some stuff around but typically isn't), `libbuiltins` is a combination of multiple translation units, if you look at the source. I'm not sure how it's turning into an object file for you and even if it

[PATCH] D51899: Remove extraneous ".a" suffix from baremetal clang_rt.builtins when compiling for baremetal.

kristina added a reviewer: t.p.northover. kristina added a comment. Added the main ARM code owner as a reviewer, I'm not entirely sure why you're getting this behavior since regression tests should have weeded it out. https://reviews.llvm.org/D51899 __

[PATCH] D51899: Remove extraneous ".a" suffix from baremetal clang_rt.builtins when compiling for baremetal.

kristina added a comment. Sorry, didn't notice this was recently added, from looking at Darwin and Android embedded tests, this doesn't really match up with either of them. The hosted tests for Android using `libbuiltins` look like this: // RUN: %clang -target arm-linux-androideabi -rtlib=com

[PATCH] D50359: Add a new library, libclang-cxx

kristina added a comment. I'm in support as long as it's a configuration option defaulting similar to LLVM's one. Should likely follow the same naming convention as LLVM, ie. `clang-shlib`. Clang has a lot of tools it ships with especially if you consider extras, I think this is one of the case

[PATCH] D50359: Add a new library, libclang-cxx

kristina added a comment. To elaborate, consider a scenario where a customer wants to build the clang driver, `diagtool` and just for the sake of the argument `change-namespace` from extras. Clang tools do not provide nearly the same level of control as most LLVM tools, so in that scenario, you

[PATCH] D51921: [VFS] vfs::directory_iterator yields path and file type instead of full Status

kristina added a comment. Added inline comments regarding code style and internal/external naming. Comment at: include/clang/Basic/VirtualFileSystem.h:135 + +public: + directory_entry() = default; The consistency is mostly related to naming of class members or

[PATCH] D51921: [VFS] vfs::directory_iterator yields path and file type instead of full Status

kristina added a comment. Fine with me, I wasn't really suggesting a revision just making a remark more or less. Sorry if it came off that way. Repository: rC Clang https://reviews.llvm.org/D51921 ___ cfe-commits mailing list cfe-commits@lists.l

[PATCH] D52066: [Driver] Fix missing MultiArch include dir on powerpcspe

kristina accepted this revision. kristina added a comment. This revision is now accepted and ready to land. LGTM. In the future please make sure you supply all patches with context (ie. `-U9`), without context they may be harder to review and more importantly `patch` may mess up. Reposito

[PATCH] D52066: [Driver] Fix missing MultiArch include dir on powerpcspe

This revision was automatically updated to reflect the committed changes. Closed by commit rC342231: [Driver] Fix missing MultiArch include dir on powerpcspe (authored by kristina, committed by ). Repository: rL LLVM https://reviews.llvm.org/D52066 Files: lib/Driver/ToolChains/Linux.cpp I

[PATCH] D52066: [Driver] Fix missing MultiArch include dir on powerpcspe

This revision was automatically updated to reflect the committed changes. Closed by commit rL342231: [Driver] Fix missing MultiArch include dir on powerpcspe (authored by kristina, committed by ). Herald added a subscriber: llvm-commits. Changed prior to commit: https://reviews.llvm.org/D52066?

[PATCH] D52093: [ToolChains] Linux.cpp: Use the same style across all all multi-arch includes. NFC.

kristina created this revision. kristina added a reviewer: rsmith. kristina added a project: clang. Herald added a subscriber: cfe-commits. Currently it seems like a mess in terms of where newlines are used. Cleaning it up so all of them use newlines and are aligned and moving the default switch

[PATCH] D43953: clangformat-vs: Fix plugin not correctly loading in some cases

kristina accepted this revision as: kristina. kristina added a comment. This revision is now accepted and ready to land. Seems something that's not easy to test or reproduce even with VS (the plugin itself is 3rd party if I understand right) but it looks sane. Repository: rC Clang https://re

[PATCH] D53102: Support for the mno-tls-direct-seg-refs flag

kristina added a comment. @nruslan This patchset is still pending review so be patient, I landed https://reviews.llvm.org/D53103 as it was reviewed and accepted by the code owner, on which this patch depends on. That said it LGTM, it's simple enough as it just forwards the argument to the backe

[PATCH] D53102: Support for the mno-tls-direct-seg-refs flag

kristina added a comment. If the author doesn't mind I can just apply the style fix after patching and then rebuild and run all the relevant tests (or would you prefer to do that?). Seems easier than a new revision for an indentation change on one line. https://reviews.llvm.org/D53102 _

[PATCH] D53102: Support for the mno-tls-direct-seg-refs flag

kristina added a comment. By the way, out of curiosity is this for anything specific (alternative libc or some user-mode-scheduling implementation)? Not nitpicking, just curious since it's an interesting topic in general and it's frustrating that the architecture is so limited in terms of regis

[PATCH] D53102: Support for the mno-tls-direct-seg-refs flag

This revision was automatically updated to reflect the committed changes. Closed by commit rC344739: Add support for -mno-tls-direct-seg-refs to Clang (authored by kristina, committed by ). Changed prior to commit: https://reviews.llvm.org/D53102?vs=169224&id=170082#toc Repository: rC Clang

[PATCH] D53475: Create ConstantExpr class

kristina added a comment. I think you may have accidentally commited the wrong patch. +struct ConstantExpr : public FullExpr { Is causing a warning right now, not sure where that came from. Repository: rC Clang https://reviews.llvm.org/D53475 ___

[PATCH] D53475: Create ConstantExpr class

kristina added a comment. Ah, it was causing this warning during build: /SourceCache/llvm-trunk-8.0/tools/clang/include/clang/AST/Expr.h:903:1: warning: 'ConstantExpr' defined as a struct here but previously declared as a class [-Wmismatched-tags] Repository: rC Clang https://reviews.llv

[PATCH] D56241: expose a control flag for interger to pointer ext warning

kristina added a comment. Going to test and land it as requested by @lebedev.ri on IRC. Repository: rC Clang CHANGES SINCE LAST ACTION https://reviews.llvm.org/D56241/new/ https://reviews.llvm.org/D56241 ___ cfe-commits mailing list cfe-commit

[PATCH] D56241: [Sema] expose a control flag for integer to pointer ext warning

This revision was automatically updated to reflect the committed changes. Closed by commit rC351082: [Sema] Expose a control flag for integer to pointer ext warning (authored by kristina, committed by ). Changed prior to commit: https://reviews.llvm.org/D56241?vs=180143&id=181585#toc Repositor

[PATCH] D54344: [Clang][CodeGen][CXX]: Workaround __attribute((no_destroy)) crash/incorrect code generation.

kristina created this revision. kristina added reviewers: rsmith, clang, JonasToth, EricWF, aaron.ballman. Herald added subscribers: dexonsmith, mehdi_amini. Herald added a reviewer: jfb. Difficult to reproduce crash, attempted it in the test, it appears to not trigger it. I can consistently repr

[PATCH] D54344: [Clang][CodeGen][CXX]: Workaround __attribute((no_destroy)) crash/incorrect code generation.

kristina updated this revision to Diff 173411. kristina added a comment. Revised (style/ordering). https://reviews.llvm.org/D54344 Files: lib/CodeGen/CGDeclCXX.cpp Index: lib/CodeGen/CGDeclCXX.cpp === --- lib/CodeGen/CGDeclCXX.

[PATCH] D54344: [Clang][CodeGen][CXX]: Workaround __attribute((no_destroy)) crash/incorrect code generation.

kristina added a comment. In https://reviews.llvm.org/D54344#1293468, @erik.pilkington wrote: > Have you tried running creduce on the preprocessed source? We should really > have a real reproducer for this, otherwise its really hard to tell what the > underlying problem is. Unable to build it

[PATCH] D54344: [Clang][CodeGen][CXX]: Workaround __attribute((no_destroy)) crash/incorrect code generation.

kristina added a comment. In https://reviews.llvm.org/D54344#1293643, @aaron.ballman wrote: > In https://reviews.llvm.org/D54344#1293468, @erik.pilkington wrote: > > > Have you tried running creduce on the preprocessed source? We should really > > have a real reproducer for this, otherwise its r

[PATCH] D54344: [Clang][CodeGen][CXX]: Workaround __attribute((no_destroy)) crash/incorrect code generation.

kristina added a comment. This seems to be a peculiar side effect of weird combinations of previous uses of attributes `no_destroy`, `used`, and `section`, as well as complex macros used to create metaclass-like systems (through the use of said attributes). It seems that there is a serious bug

[PATCH] D54373: [clang]: Correct naming conventions and 80 col rule violation in CGDeclCXX.cpp. NFC.

kristina created this revision. kristina added a reviewer: clang. Herald added a subscriber: cfe-commits. Noticed while working with that area of code, NFC. Repository: rC Clang https://reviews.llvm.org/D54373 Files: lib/CodeGen/CGDeclCXX.cpp Index: lib/CodeGen/CGDeclCXX.cpp

[PATCH] D54373: [clang]: Correct naming conventions and 80 col rule violation in CGDeclCXX.cpp. NFC.

This revision was automatically updated to reflect the committed changes. Closed by commit rL346582: Correct naming conventions and 80 col rule violation in CGDeclCXX.cpp. NFC. (authored by kristina, committed by ). Herald added a subscriber: llvm-commits. Changed prior to commit: https://revie

[PATCH] D54344: [Clang][CodeGen][CXX]: Workaround __attribute((no_destroy)) crash/incorrect code generation.

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)

[PATCH] D54344: [Clang][CodeGen][CXX]: Workaround __attribute((no_destroy)) crash/incorrect code generation.

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

[PATCH] D54344: [Clang][CodeGen][CXX]: Workaround __attribute((no_destroy)) crash/incorrect code generation.

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 ===

[PATCH] D54379: Add Hurd support

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())

[PATCH] D54379: Add Hurd support

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

[PATCH] D54344: [Clang][CodeGen][CXX]: Workaround __attribute((no_destroy)) crash/incorrect code generation.

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

[PATCH] D53263: Fix places where the return type of a FunctionDecl was being used in place of the function type

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

[PATCH] D54344: [Clang][CodeGen][CXX]: Workaround __attribute((no_destroy)) crash/incorrect code generation.

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 =

[PATCH] D53263: Fix places where the return type of a FunctionDecl was being used in place of the function type

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

[PATCH] D53263: Fix places where the return type of a FunctionDecl was being used in place of the function type

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

[PATCH] D54344: [Clang][CodeGen][CXX]: Workaround __attribute((no_destroy)) crash/incorrect code generation.

kristina added a comment. In https://reviews.llvm.org/D54344#1294942, @erik.pilkington wrote: > LGTM too, with one small fix in test. Thanks for working on this! Well, I figured since the assertion being tripped was (before investigation) the only reliable way to notice the bug it makes sense

[PATCH] D54344: [Clang][CodeGen][CXX]: Workaround __attribute((no_destroy)) crash/incorrect code generation.

kristina updated this revision to Diff 173602. kristina added a comment. Revised to address comments. 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 =

[PATCH] D54344: [Clang][CodeGen][CXX]: Workaround __attribute((no_destroy)) crash/incorrect code generation.

kristina added a comment. Though on the other hand it makes no sense to skip it with asserts off either, that just clicked in my head, sorry. Repository: rC Clang https://reviews.llvm.org/D54344 ___ cfe-commits mailing list cfe-commits@lists.llv

[PATCH] D54344: [Clang][CodeGen][CXX]: Workaround __attribute((no_destroy)) crash/incorrect code generation.

This revision was automatically updated to reflect the committed changes. Closed by commit rL346628: [CodeGen][CXX]: Fix no_destroy CG bug under specific circumstances (authored by kristina, committed by ). Herald added a subscriber: llvm-commits. Changed prior to commit: https://reviews.llvm.o

[PATCH] D54379: Add Hurd toolchain support to Clang

kristina added reviewers: aaron.ballman, erik.pilkington, rsmith. kristina added a comment. Alright, once this is fully reviewed, if accepted, I can land the LLVMSupport change and add your new target and close the stack. In the meantime, adding some more reviewers who have more experience with

[PATCH] D54379: Add Hurd toolchain support to Clang

kristina added a comment. Added first batch of comments regarding the patch. Some style, some semantics-related. Comment at: lib/Basic/Targets/OSTargets.h:283 +Builder.defineMacro("__GLIBC__"); +Builder.defineMacro("__ELF__"); +if (Opts.POSIXThreads) --

[PATCH] D54379: Add Hurd toolchain support to Clang

kristina added inline comments. Comment at: lib/Driver/Driver.cpp:392 +static void replaceString(std::string &S, + const char *Other, Same as previous comment regarding this type of function. Repository: rC Clang https://reviews.ll

[PATCH] D54379: Add Hurd toolchain support to Clang

kristina added a comment. Phab lost this inline command for some reason, but please leave some comment regarding why that part in `Driver.cpp` does what it does (summarize the conclusion of the discussion in https://reviews.llvm.org/D54378). Comment at: lib/Driver/Driver.cpp:

[PATCH] D54379: Add Hurd toolchain support to Clang

kristina added a comment. The other lost comment was regarding the functions where you're using `strcpy` instead of idiomatic LLVM and also creating unnecessary temporary `std::string` instances on the stack. Repository: rC Clang https://reviews.llvm.org/D54379 __

[PATCH] D54379: Add Hurd toolchain support to Clang

kristina added a comment. Commented on that particular idiom, there's two instances where it's used, aside from variable naming issues (`pos` should be `Pos`) it's very non idiomatic as far as rest of LLVM code goes, don't pass string literals around as `const char*`, prefer `StringRef` instead

[PATCH] D54379: Add Hurd toolchain support to Clang

kristina added inline comments. Comment at: lib/Driver/Driver.cpp:418 + replaceString(T, "-pc-gnu", "-pc-hurd-gnu"); + TargetTriple = T; + Reference to a local variable? Repository: rC Clang https://reviews.llvm.org/D54379 __

[PATCH] D54379: Add Hurd toolchain support to Clang

kristina added inline comments. Comment at: lib/Driver/Driver.cpp:418 + replaceString(T, "-pc-gnu", "-pc-hurd-gnu"); + TargetTriple = T; + kristina wrote: > Reference to a local variable? Hm, actually this is fine I guess, just avoid `strlen` and pass literals

[PATCH] D54379: Add Hurd toolchain support to Clang

kristina added a comment. Also, this needs unit tests and FileCheck tests. Repository: rC Clang https://reviews.llvm.org/D54379 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D54379: Add Hurd toolchain support to Clang

kristina added a comment. As discussed in `#hurd`, a few additional comments. The Hurd codepath should first check if the triple is eligible, ie. minimizing any cost to the driver invocation for most targets, which are not going to be Hurd. In fact I would wrap it in `LLVM_UNLIKELY` but that's

[PATCH] D54379: Add Hurd toolchain support to Clang

kristina added a comment. I'll re-review when I'm up, from a quick glance it looks much better but I'll have to patch it over my fork and try out a few things (Mostly x86_64 Linux and Darwin test suites). I think the test is lacking a bit, there's a lot of stuff that isn't covered, and there's

[PATCH] D53696: [Haiku] Support __float128 for Haiku x86 and x86_64

kristina added a reviewer: kristina. kristina added a comment. Do you mind resubmitting those with context? Also I would suggest asking Tom Stellard as he's in charge of handling cherrypicking patches to go into releases once the major rolls over and I think we're pretty close (?) to 7.0.1. Re

[PATCH] D53696: [Haiku] Support __float128 for Haiku x86 and x86_64

kristina added a comment. Linux, Solaris and OpenBSD also define it where appropriate, it depends on the target. This is why it's much easier to review when full context (diff with `-U 9`) is provided, and means that it has a much lower change of `patch` doing something unexpected. Reposi

[PATCH] D53696: [Haiku] Support __float128 for Haiku x86 and x86_64

kristina requested changes to this revision. kristina added a comment. In https://reviews.llvm.org/D53696#1305990, @kallisti5 wrote: > Indeed. I agree the _GLIBCXX_USE_FLOAT128 is misplaced there. That comes > from config.h in libstdc++. That's working around an issue in Haiku's build > (lib

[PATCH] D54379: Add Hurd toolchain support to Clang

kristina added a comment. Ping. Would like someone else to co-review this, everything looks fine aside from tests (I think a portion of this could use a short unit test with regards to creating the actual target instance). Also your FileCheck test is only for invoking `clang -cc1` from the look

[PATCH] D53696: [Haiku] Support __float128 for Haiku x86 and x86_64

kristina added a comment. Ping. Is the author still around? I'm happy to take over this revision if not, and add `__FLOAT128__` but unless someone says `_GLIBCXX_USE_FLOAT128` is actually defined by the toolchain, I'll remove that line, it looks like it really shouldn't be defined in the toolch

[PATCH] D54379: Add Hurd toolchain support to Clang

kristina added a comment. Alright, will patch it into my fork in a bit and try it out, if everything looks good, I'll land the stack. (Again huge sorry about this taking some time, have lots on my hands at the moment) Repository: rC Clang CHANGES SINCE LAST ACTION https://reviews.llvm.org

[PATCH] D53696: [Haiku] Support __float128 for Haiku x86 and x86_64

kristina commandeered this revision. kristina edited reviewers, added: return; removed: kristina. kristina added a comment. New revision in D54901 . Repository: rC Clang CHANGES SINCE LAST ACTION https://reviews.llvm.org/D53696/new/ https://reviews.llvm.org

[PATCH] D54901: [Haiku] Support __float128 for Haiku x86 and x86_64

kristina accepted this revision. kristina added a comment. This revision is now accepted and ready to land. LGTM. (Revision of D53696 ) Repository: rC Clang CHANGES SINCE LAST ACTION https://reviews.llvm.org/D54901/new/ https://reviews.llvm.org/D54901 __

[PATCH] D54901: [Haiku] Support __float128 for Haiku x86 and x86_64

kristina added a comment. Let me know if you need this committed. Comment at: lib/Basic/Targets/OSTargets.h:260 DefineStd(Builder, "unix", Opts); +if (this->HasFloat128) { + Builder.defineMacro("__FLOAT128__"); One tiny style-related nitpick, no n

[PATCH] D54379: Add Hurd toolchain support to Clang

kristina added a comment. Your tests don't pass: Testing Time: 21.51s Failing Tests (1): Clang :: Driver/hurd.c Expected Passes: 470 Expected Failures : 3 Unsupported Tests : 71 Unexpected Failures: 1 Seems to be an

[PATCH] D54379: Add Hurd toolchain support to Clang

kristina added a comment. Hm, yes you're right, the files aren't in the diff that Phab gives me, this is annoying. I wonder if it's stripping them from the diff for some reason, can you upload the raw diff? Repository: rC Clang CHANGES SINCE LAST ACTION https://reviews.llvm.org/D54379/new

[PATCH] D54379: Add Hurd toolchain support to Clang

kristina accepted this revision as: kristina. kristina added a comment. Actually nevermind, I'll just create them myself, seems like a strange bug. Repository: rC Clang CHANGES SINCE LAST ACTION https://reviews.llvm.org/D54379/new/ https://reviews.llvm.org/D54379 ___

[PATCH] D54379: Add Hurd toolchain support to Clang

kristina accepted this revision. kristina added a comment. This revision is now accepted and ready to land. Yes everything passes now, my bad for not noticing. Repository: rC Clang CHANGES SINCE LAST ACTION https://reviews.llvm.org/D54379/new/ https://reviews.llvm.org/D54379 ___

[PATCH] D54379: Add Hurd toolchain support to Clang

This revision was automatically updated to reflect the committed changes. Closed by commit rC347833: Add Hurd target to Clang driver (2/2) (authored by kristina, committed by ). Repository: rC Clang CHANGES SINCE LAST ACTION https://reviews.llvm.org/D54379/new/ https://reviews.llvm.org/D543

[PATCH] D54901: [Haiku] Support __float128 for Haiku x86 and x86_64

This revision was automatically updated to reflect the committed changes. Closed by commit rL348368: [Haiku] Support __float128 for x86 and x86_64 (authored by kristina, committed by ). Herald added a subscriber: llvm-commits. Changed prior to commit: https://reviews.llvm.org/D54901?vs=175266&i

[PATCH] D55150: Emit warnings from the driver for use of -mllvm or -Xclang options.

kristina added a comment. Personally I'm against this type of warning as it's likely anyone using `-mllvm` is actually intending to adjust certain behaviors across one or more passes with a lot of switches supported by it being intentionally hidden from ` --help` output requiring explicitly spe

[PATCH] D55150: Emit warnings from the driver for use of -mllvm or -Xclang options.

kristina added a comment. > There is a cost to having people encode these flags into their build systems > -- it can then cause issues if we ever change these internal flags. I do not > think any Clang maintainer intends to support these as stable APIs, unlike > most of the driver command-line.

[PATCH] D55150: Emit warnings from the driver for use of -mllvm or -Xclang options.

kristina added a comment. In D55150#1321734 , @thakis wrote: > I don't have an opinion on this patch (if you force me to have one, I'm > weakly in favor), but I agree with the general sentiment. When I told people > to not use mllvm and Xclang before, th

[PATCH] D55150: Emit warnings from the driver for use of -mllvm or -Xclang options.

kristina requested changes to this revision. kristina added a comment. In D55150#1321829 , @efriedma wrote: > I'm not sure that putting a warning that can be disabled really helps here; > anyone who needs the option will just disable the warning anyway, a

[PATCH] D17741: adds __FILE_BASENAME__ builtin macro

kristina added a comment. If the author is still missing at the end of next week, any objections to me resubmitting a similar patch that just implements `__FILE_NAME__` or `__BASE_NAME__` (Need a few more opinions here I guess, personally I think `__FILE_NAME__` makes more sense)? I'll carve i

[PATCH] D17741: adds __FILE_BASENAME__ builtin macro

kristina commandeered this revision. kristina added a reviewer: weimingz. kristina added a comment. Sorry, forgot about this, will make a new diff with just the macro for review later tonight. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D17741/new/ https://reviews.llvm.org/D17741 _

[PATCH] D61756: Add a __FILE_NAME__ macro.

kristina created this revision. kristina added reviewers: aaron.ballman, rsmith, rnk. Herald added a project: clang. A much simplified version of D17741 which adds a new builtin macro `__FILE_NAME__` that is similar to `__FILE__` but only renders the last path c

[PATCH] D17741: adds __FILE_BASENAME__ builtin macro

kristina abandoned this revision. kristina added a comment. Superseded by D61756 CHANGES SINCE LAST ACTION https://reviews.llvm.org/D17741/new/ https://reviews.llvm.org/D17741 ___ cfe-commits mailing list cfe-com

[PATCH] D61756: Add a __FILE_NAME__ macro.

kristina updated this revision to Diff 198894. kristina added a comment. Fix style, remove unnecessary braces, add missing newline. Repository: rC Clang CHANGES SINCE LAST ACTION https://reviews.llvm.org/D61756/new/ https://reviews.llvm.org/D61756 Files: include/clang/Lex/Preprocessor.h

[PATCH] D52093: [ToolChains] Linux.cpp: Use the same style across all all multi-arch includes. NFC.

kristina added a comment. Yeah but what about the rest of them, most are completely on their own line, some no newlines, mipsel64 was split across two lines while others weren't, and switch is meant to have default as the topmost case IIRC. I mean I'm if you don't think it's more readable that

[PATCH] D52151: Also manages clang-X as tool for scan-build

kristina added a comment. Nitpick: This regex is far too broad to cover the rare use case where you'd be invoking clang as `clang-N`. I think something like `clang(?:\-[789])?` would be more suitable? Repository: rC Clang https://reviews.llvm.org/D52151 __

[PATCH] D45741: Python bindings: Fix handling of file bodies with multi-byte characters

kristina added a comment. Would you mind re-uploading these patches with full context (with `diff -U9`). 3 lines of context around changes makes this very difficult to review. Also I would suggest testing for Python version and using appropriate semantics (using `encode` in case of Python3)

[PATCH] D52151: Also manages clang-X as tool for scan-build

kristina added a comment. Just a minor suggestion, I think it would make it more clear as before LLVM 7, Clang did not have a version number with the main executable. GCC is slightly less consistent with their formats as they usually ship as host compilers with various suffixes, but with Clang

  1   2   >