ruiu added inline comments.
Comment at: lib/Driver/Tools.cpp:234
const ArgList , ArgStringList ,
const JobAction ) {
const Driver = TC.getDriver();
hans wrote:
> Yes, this doesn't seem like exactly
ruiu added inline comments.
Comment at: lib/Driver/Tools.cpp:284
+ // propagate -fdiagnostics-color.
+ if (StringRef(TC.GetLinkerPath()).endswith("ld.lld") &&
+ D.getDiags().getShowColors())
rsmith wrote:
> I don't think this will work for
ruiu added a comment.
Nico suggested I make change to CMake instead of Clang so that CMake adds
-Wl,-color-diagnostics if it detects the linker can accept that option. I think
that makes sense, so I'll look into it.
https://reviews.llvm.org/D27603
ruiu added a comment.
Is this a minimal test case that can produce the issue? It'd be awesome if you
can reduce it.
sema-segvcheck.c is not a good name for this test because that name can be used
for any crash bug. You want to see other files in the same directory to name
your file so that
ruiu added a comment.
I wonder if this test is actually fragile. How can you break this test by
adding a new flag?
Comment at: clang/test/Driver/autocomplete.c:3
+// If this test had broke due to adding/modifying flags or changing HelpText,
+// please modify this file
ruiu added a comment.
So, does this comment actually make sense? Looks like you cannot break this
test by adding or modifying flags or by changing HelpText. (Theoretically,
it'll break if you remove -fsyntax-only, for example, but that is not
realistic.)
https://reviews.llvm.org/D36209
ruiu accepted this revision.
ruiu added a comment.
LGTM
https://reviews.llvm.org/D36209
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
ruiu added a comment.
Hans already merged it in r310723.
Repository:
rL LLVM
https://reviews.llvm.org/D36567
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
ruiu added a comment.
Yuka,
You seem to have committed a release note for 5.0 to SVN trunk. What you
should've done is to do that to the 5.0 branch. I'll correct the error for you.
Repository:
rL LLVM
https://reviews.llvm.org/D36567
___
ruiu added a comment.
This patch allows us to embed a piece of C++ code to each command line option
to construct a list of argument candidates at runtime. With this patch, .inc
files generated by OptParserEmitter contain C macros that in turn include other
.inc files. That is a flexible
ruiu accepted this revision.
ruiu added a comment.
This revision is now accepted and ready to land.
LGTM
https://reviews.llvm.org/D35448
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
ruiu added inline comments.
Comment at: clang/include/clang/Basic/DiagnosticIDs.h:266
+ static std::vector getDiagnosticFlags();
+
Please write a function comment just like other member functions in this class.
Comment at:
ruiu accepted this revision.
ruiu added a comment.
LGTM
Comment at: clang/lib/Driver/Driver.cpp:1283
+ for (StringRef S : DiagnosticIDs::getDiagnosticFlags())
+if (StringRef(S).startswith(PassedFlags))
+ SuggestedCompletions.push_back(S);
ruiu added inline comments.
Comment at: clang/lib/Driver/Driver.cpp:1302
-llvm::outs() << llvm::join(SuggestedCompletions, " ") << '\n';
+llvm::outs() << llvm::join(SuggestedCompletions, "\n") << '\n';
return false;
Now that the separator and the
ruiu added inline comments.
Comment at: clang/lib/Driver/Driver.cpp:1303
+ llvm::outs() << S << "\n";
+llvm::outs() << "\n";
return false;
You want to print out just one '\n' at end instead of two, no?
https://reviews.llvm.org/D35759
ruiu added inline comments.
Comment at: clang/utils/bash-autocomplete.sh:60
eval local path=${COMP_WORDS[0]}
- flags=$( "$path" --autocomplete="$arg" 2>/dev/null )
+ flags=$( "$path" --autocomplete="$arg" 2>/dev/null | sed -e 's/\t.*.\s*$//' )
# If clang is old that it
ruiu accepted this revision.
ruiu added a comment.
This revision is now accepted and ready to land.
LGTM
So, this patch changes the format of the --autocomplete option in an
incompatible way. The bash completion script that will be shipped with LLVM 5.0
will not be able to read the output of
ruiu accepted this revision.
ruiu added a comment.
This revision is now accepted and ready to land.
LGTM
https://reviews.llvm.org/D35763
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
ruiu added a comment.
Looks like this demangler's design is similar to my demangler for Microsoft
name mangling scheme (https://reviews.llvm.org/D34667). Is that a coincidence?
Both demanglers create AST, stringize it using print_left/print_right (I named
them write_pre/write_post), and use
ruiu added inline comments.
Comment at: clang/utils/bash-autocomplete.sh:3
+
+_clang_filedir()
+{
yamaguchi wrote:
> ruiu wrote:
> > Is the output of `compgen -f` the same as `_filedir`? If so, can you always
> > use `compgen -f`?
> _filedir is better than
ruiu added inline comments.
Comment at: clang/utils/bash-autocomplete.sh:8-9
+ # the latter doesn't. So we use compgen only when _filedir is not provided.
+ _filedir 2>/dev/null
+ [[ "$?" != 0 ]] && COMPREPLY=( $( compgen -f ) )
+}
If this works, you can
ruiu accepted this revision.
ruiu added a comment.
This revision is now accepted and ready to land.
LGTM
https://reviews.llvm.org/D34924
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
ruiu added inline comments.
Comment at: clang/utils/bash-autocomplete.sh:3
+
+_clang_filedir()
+{
Is the output of `compgen -f` the same as `_filedir`? If so, can you always use
`compgen -f`?
Comment at: clang/utils/bash-autocomplete.sh:7
+
ruiu accepted this revision.
ruiu added a comment.
LGTM. Confirmed that it works as expected on my Mac which doesn't have the
bash-autocomplete package.
Comment at: clang/utils/bash-autocomplete.sh:19
+COMPREPLY=()
+cword=$COMP_CWORD
+cur="${COMP_WORDS[$cword]}"
ruiu added inline comments.
Comment at: clang/utils/bash-autocomplete.sh:37
arg="$w1=,"
+ elif [[ ${cur:0:1} == '-' && ${cur: -1} == '=' ]]; then
+# -foo=
I think you can do `"$cur" == -*=`.
https://reviews.llvm.org/D34927
ruiu added inline comments.
Comment at: lib/Basic/Targets.cpp:6553
+class MicrosoftARM64leTargetInfo
+: public WindowsTargetInfo {
I cannot imagine there will be MicrosoftARM64beTargetInfo, so I wonder if we
should name this MicrosoftARM64TargetInfo.
ruiu accepted this revision.
ruiu added a comment.
LGTM
https://reviews.llvm.org/D34761
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
ruiu added a comment.
Please add this to https://reviews.llvm.org/D32509. I'll commit this and that
as one patch.
Repository:
rL LLVM
https://reviews.llvm.org/D32565
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
ruiu added inline comments.
Comment at: test/Sema/warn-missing-braces.c:6-11
+typedef struct _GUID {
+ unsigned Data1;
+ unsigned short Data2;
+ unsigned short Data3;
+ unsigned char Data4[8];
+} GUID;
You can simplify this, can't you? It seems something
ruiu added inline comments.
Comment at: lib/Sema/SemaInit.cpp:875
if (!VerifyOnly) {
StructuredSubobjectInitList->setType(T);
Is it intentional that you run the new code only when !VerifyOnly?
Comment at:
ruiu added inline comments.
Comment at: lib/Sema/SemaInit.cpp:897
+SemaRef.getSourceManager().isInSystemHeader(SpellingLoc))
+ return;
SemaRef.Diag(StructuredSubobjectInitList->getLocStart(),
Please use clang-format to format your
ruiu added inline comments.
Comment at: llvm/utils/TableGen/OptParserEmitter.cpp:308
const Record = *Opts[I];
if (!isa(R.getValueInit("ValuesCode"))) {
OS << "{\n";
You can flip the condition to do early continue.
Comment at:
ruiu added inline comments.
Comment at: clang/include/clang/Driver/Options.td:2254
+ ValuesCode<[{
+const char* Values =
+#define LANGSTANDARD(id, name, lang, desc, features) name ","
ruiu wrote:
> I think Raphael suggested indenting embedded code with at least
ruiu accepted this revision.
ruiu added a comment.
LGTM
https://reviews.llvm.org/D33383
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
ruiu added inline comments.
Comment at: clang/include/clang/Driver/Options.td:496
def cl_std_EQ : Joined<["-"], "cl-std=">, Group,
Flags<[CC1Option]>,
- HelpText<"OpenCL language standard to compile for.">;
+ HelpText<"OpenCL language standard to compile for.">,
ruiu created this revision.
Currently, autocompleted options are displayed in the same order as we
wrote them in .td files. This patch sort them out in clang so that they
are sorted alphabetically. This should improve usability.
https://reviews.llvm.org/D34557
Files:
This revision was automatically updated to reflect the committed changes.
Closed by commit rL306116: Sort the autocomplete candidates before printing
them out. (authored by ruiu).
Changed prior to commit:
https://reviews.llvm.org/D34557?vs=103731=103733#toc
Repository:
rL LLVM
ruiu added a comment.
I'd copy what Hal mentioned in other review thread for other GSoC project. You
don't want to tag your patches with "[GSoC]" because it doesn't describe
anything about patch contents and many other unrelated patches could have been
tagged as single "[GSoC]" tag. Instead,
ruiu added inline comments.
Comment at: clang/utils/bash-autocomplete.sh:28
+ flags=$( clang --autocomplete="$arg" 2>/dev/null )
+ # Check if --autocomplete is supported in user's clang version.
+ if [[ "$?" != 0 ]]; then
It is probably a bit better if you
ruiu accepted this revision.
ruiu added a comment.
This revision is now accepted and ready to land.
LGTM
https://reviews.llvm.org/D34607
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
ruiu added inline comments.
Comment at: clang/utils/bash-autocomplete.sh:10
+ do
+arg="$arg""${COMP_WORDS[$i]},"
+ done
ruiu wrote:
> nit: you don't need double double-quotes. Instead, `"$arg${COMP_WORDS[$i]},"
> should just work.
On second thought, I
ruiu added a comment.
As to the order of Command variable contents, I think I'm not convinced. You
can access the last element as `Command[Command.size() - 1]` (or maybe
`Command.back()`), no? It is slightly awkward than `Command[0]`, but that's not
horribly hard to handle, and passing
ruiu added a comment.
Overall looking good. Good work! A few minor comments.
Comment at: clang/include/clang/Driver/Options.td:2198
def stdlib_EQ : Joined<["-", "--"], "stdlib=">, Flags<[CC1Option]>,
- HelpText<"C++ standard library to use">;
+ HelpText<"C++ standard
ruiu added a comment.
Yuka,
This is beautiful. Overall looking pretty good. Some minor stylistic comments.
Comment at: clang/include/clang/Driver/Options.h:43
+#define OPTION(PREFIX, NAME, ID, KIND, GROUP, ALIAS, ALIASARGS, FLAGS, PARAM,
\
+ HELPTEXT, METAVAR,
ruiu added a comment.
I'm totally against adding per-OS path knowledge to our linker. Compilers
already know include paths and I don't want to maintain another list of paths
in the linker. Also this can be more confusing than useful when you are doing
cross-linking.
For all OSes other than
ruiu accepted this revision.
ruiu added a comment.
LGTM. I don't think I have enough knowledge to sign off, but as Saleem has
already LGTM'ed, I guess it should be okay.
https://reviews.llvm.org/D34706
___
cfe-commits mailing list
ruiu accepted this revision.
ruiu added a comment.
This revision is now accepted and ready to land.
LGTM
https://reviews.llvm.org/D37943
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
ruiu added a comment.
This patch virtually sets `ld64` the linker command name for macOS. I'd be a
bit reluctant doing that, because `ld64` sounds like a too generic name to
indicate "a linker for macOS". But that's probably okay because we don't have a
better name.
Can you get an LGTM from
ruiu added a comment.
Is this for cross-compilation? If you invoke lld on macOS, it should act as
ld64.
macOS lld is not actively maintained, so I wouldn't make it available more
widely.
Repository:
rL LLVM
https://reviews.llvm.org/D38290
___
ruiu added a comment.
Technically, this patch fixes your issue, but I don't think this is a good way
of fixing it because `parse()` function now has an internal state which is not
obvious to the user of the function. Can you review
https://reviews.llvm.org/D37217? This is an alternative fix
ruiu accepted this revision.
ruiu added a comment.
This revision is now accepted and ready to land.
LGTM
https://reviews.llvm.org/D36820
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
ruiu added inline comments.
Comment at: llvm/utils/TableGen/OptParserEmitter.cpp:314
for (const std::string : R.getValueAsListOfStrings("Prefixes")) {
-OS << "bool ValuesWereAdded = ";
+OS << "ValuesWereAdded = ";
OS << "Opt.addValues(";
ruiu added inline comments.
Comment at: clang/include/clang/Driver/CC1Options.td:104
+ ValuesCode<[{
+const char* Values =
+#define GET_CHECKERS
`const char* Values` -> `const char *Values`
Comment at:
ruiu added a comment.
Yes, specifically, lld does string tail merging (instead of regular string
merging) when -O2 is given, so it would produce slightly smaller outputs.
Repository:
rL LLVM
https://reviews.llvm.org/D39017
___
cfe-commits
ruiu added a comment.
I think this is the right thing to do, but I'd defer it to libunwind's owner to
approve the patch.
https://reviews.llvm.org/D39918
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
ruiu added a comment.
Actually I don't have a strong opinion on that topic. It seems like just
truncating the section name to ".eh_fram" at the linker is good enough, but how
much important is the compatibility with GNU ld?
https://reviews.llvm.org/D39918
ruiu accepted this revision.
ruiu added a comment.
LGTM
Comment at: lib/Driver/Driver.cpp:1204
+return X < 0;
+ return A.compare(B) > 0; });
I believe that if you use clang-format, `});` will be put to a separate line
like this.
ruiu added a comment.
Maybe we should do case-insensitive string comparison first, and if two strings
are considered the same, try again in case-sensitive manner? Otherwise, even
though the output is now deterministic, the output order is still dependent on
the order of input strings.
ruiu added a comment.
Perhaps, this is a bit more straightforward.
if (int X = A.compare_lower(B))
return X < 0;
return A.compare(B) < 0;
https://reviews.llvm.org/D40234
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
ruiu added a comment.
(I found that information at
http://wiki.bash-hackers.org/scripting/bashchanges#quoting_expansions_substitutions_and_related)
Repository:
rC Clang
https://reviews.llvm.org/D47273
___
cfe-commits mailing list
ruiu added a comment.
Looks like $'' is there since bash 2.0 which should be pretty safe to use in
the script. So feel free to use $'' instead of $(echo ...) in this patch.
Repository:
rC Clang
https://reviews.llvm.org/D47273
___
cfe-commits
ruiu accepted this revision.
ruiu added a comment.
This revision is now accepted and ready to land.
LGTM
https://reviews.llvm.org/D47273
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
ruiu added a comment.
Oh, I didn't know the existence of $''. Thank you for the suggestion! I wonder
which version of bash started supporting that notation. Do you know if all
recent versions of bash support it? Unfortunately `$'' bash` is very hard query
to google...
Repository:
rC Clang
ruiu added a comment.
Even though Perl may be installed to 99.99% of machines that use this
autocomplete script, using perl instead of sed is too much. If we could use
perl, we'd have wrote this script entirely in perl in the first place. We
shouldn't add a dependency to perl.
I wonder if you
ruiu added a comment.
How about
sed -e "$(echo -e 's/\t.*//')"
?
Repository:
rC Clang
https://reviews.llvm.org/D47273
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
ruiu accepted this revision.
ruiu added a comment.
This revision is now accepted and ready to land.
Well, I don't think that pointing out that we shouldn't convert strings between
UTF8 and UTF16 back and forth is nitpicking, but yeah, this has already been
addressed, so feel free to submit.
ruiu added inline comments.
Comment at: llvm/lib/Support/Windows/Process.inc:227
return mapWindowsError(GetLastError());
- if (Length > LongPath.capacity()) {
+ if (static_cast(Length) > MAX_PATH) {
// We're not going to try to deal with paths longer than MAX_PATH,
ruiu added inline comments.
Comment at: llvm/lib/Support/Windows/Process.inc:216
+ wchar_t ModuleName[MAX_PATH];
+ int Length = ::GetModuleFileNameW(NULL, ModuleName, MAX_PATH);
+ if (Length == 0 || Length == MAX_PATH) {
Can't this be size_t?
ruiu added inline comments.
Comment at: llvm/lib/Support/Windows/Process.inc:240
+ Filename.assign(Base.begin(), Base.end());
+ return ec;
}
The intention of the code is to return a success, so it is less confusing if
you directly return a success (i.e.
ruiu added inline comments.
Comment at: llvm/lib/Support/Windows/Process.inc:226
+
+static std::error_code GetLongArgv0Path(const wchar_t *Argv0,
+SmallVectorImpl ,
It looks like this function is a bit too complicated. I'd
ruiu added a comment.
LGTM
Repository:
rL LLVM
https://reviews.llvm.org/D47669
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
ruiu added a comment.
It seems you converted the same string back and forth between UTF-8 and UTF-16
to call Windows functions and LLVM functions. That's not only a waste of time
(which is not a big problem) but complicates the code.
I'd define `std::error GetExecutableName(std::string )`
ruiu added a comment.
So the best practice is, when you get a UTF-16 string from an Windows API, you
should convert it to UTF-8 as soon as you can so that you can always process
strings as UTF-8. Likewise, you should always keep all string in UTF-8 in
memory and convert them to UTF-16 just
ruiu added a comment.
It seems like you are trying a bit too hard to keep the original code which is
not always a good idea to keep the clarity of the code.
So, IIUC, you this function:
- returns a full filename of the current executable. That can be written using
GetModuleFileName and
ruiu added a comment.
Looks like this patch contains two unrelated changes. Please separate your
change from the lit config changes.
https://reviews.llvm.org/D47578
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
ruiu added a comment.
> I also wonder which is better `#pragma comment(lib, "m")` or `#pragma
> comment(lib, "m")`.
Sorry, I meant `#pragma comment(lib, "m")` or `#pragma comment("lib", "m")`.
Repository:
rC Clang
https://reviews.llvm.org/D42758
ruiu added a comment.
`#pragma comment` is what Microsoft is using, but is everybody happy about that
name? It isn't clear at least to me that what it actually means is linker
directives.
I also wonder which is better `#pragma comment(lib, "m")` or `#pragma
comment(lib, "m")`.
Repository:
ruiu added a comment.
> That's weird, because lots of lldb tests compile and link test binaries on
> Windows with `-fuse-ld=lld` (without the `.exe`). What makes you say the
> `.exe` is necessary?
Maybe he is using clang (not clang-cl) and want to use ld.lld.exe instead of
lld-link?
ruiu added a comment.
This change looks good to me, but I think we shouldn't check in an executable
binary file. Can you create `ld.foo.exe` in the test? Since you don't actually
run ld.foo.exe, creating that executable should be very easy.
https://reviews.llvm.org/D43621
ruiu added a comment.
> The Fuchsia driver relies on lld so invoke clang with -fuse-ld=lld. This gets
> the test passing when the clang default linker is something other than lld.
Does it work if lld is not installed at all? I believe if the driver cannot
find a specified linker, it reports an
ruiu accepted this revision.
ruiu added a comment.
This revision is now accepted and ready to land.
LGTM!
Repository:
rC Clang
https://reviews.llvm.org/D50395
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
ruiu added a comment.
If this piece of code used to be working correctly, there is another piece of
code that adds `-flavor ld` to the command line. But if that's the case, this
change wouldn't work because it constructs something like "ld.lld -flavor ld
...". ld.lld doesn't accept `-flavor`
ruiu accepted this revision.
ruiu added a comment.
This revision is now accepted and ready to land.
LGTM
Repository:
rC Clang
https://reviews.llvm.org/D51358
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
This revision was automatically updated to reflect the committed changes.
Closed by commit rC330067: Use InitLLVM in clang as well. (authored by ruiu,
committed by ).
Herald added a subscriber: cfe-commits.
Changed prior to commit:
https://reviews.llvm.org/D45634?vs=142462=142463#toc
ruiu accepted this revision.
ruiu added a comment.
This revision is now accepted and ready to land.
LGTM
https://reviews.llvm.org/D53589
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
ruiu added a comment.
lld's driver is intentionally designed to be agnostic of the host that the
linker is running for the reasons described at the beginning of Driver.cpp:
https://github.com/llvm-project/lld/blob/master/ELF/Driver.cpp#L13 I think I
like that approach. If you need to do this,
ruiu accepted this revision.
ruiu added a comment.
This revision is now accepted and ready to land.
Let me make it clear again that I'm *not* okay with this approach. Please
explicitly pass command line arguments from the compiler driver to the linker.
CHANGES SINCE LAST ACTION
ruiu accepted this revision.
ruiu added a comment.
This revision is now accepted and ready to land.
LGTM
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D55443/new/
https://reviews.llvm.org/D55443
___
cfe-commits mailing list
ruiu added a comment.
I thought NetBSD's tar is bsdtar because it's BSD... Anyways, I think I'm fine
with this change, as the new test (which matches both `foo\\.o` and `foo\.o`)
does not match a string that we don't want it to match.
Repository:
rLLD LLVM Linker
CHANGES SINCE LAST ACTION
ruiu added inline comments.
Comment at: test/lit.cfg.py:99
+stdout=subprocess.PIPE,
+stderr=subprocess.PIPE,
+env={'LANG': 'C'})
MaskRay wrote:
> If you don't need stderr, remove `stderr=subprocess.PIPE,`
>
> `subprocess.Popen followed
ruiu added inline comments.
Comment at: test/lit.cfg.py:99
+stdout=subprocess.PIPE,
+stderr=subprocess.PIPE,
+env={'LANG': 'C'})
ruiu wrote:
> mgorny wrote:
> > MaskRay wrote:
> > > If you don't need stderr, remove
ruiu added inline comments.
Comment at: test/lit.cfg.py:100
config.available_features.add('gnutar')
-tar_version.wait()
Maybe a silly question, but is this OK to remove this line?
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D55443/new/
ruiu accepted this revision.
ruiu added a comment.
This revision is now accepted and ready to land.
LGTM
We might be able to change NetBSD's tar but I guess that takes very long time.
This test is not very important in the sense that this tests just test a corner
case. So I think we should
ruiu accepted this revision.
ruiu added a comment.
LGTM. Please commit.
Peter, I wonder if you are fine with the default 64KiB page size with lld,
especially given that lld always round up the text segment size to the maximum
page size on disk and fill the padding with trap instructions. On
ruiu added a comment.
+peter.smith
Somewhat tangent to this patch, but is 64KiB a reasonable default for `-z
max-page-size`? I believe that max-page-size is set to 64KiB so that OS/CPU
whose minimum page size is 64KiB can load an executable linked with lld, but is
there any real target OS/CPU
ruiu added a comment.
I wouldn't rush to submit this now, given that this issue is not new at all.
Maybe we can just wait for Peter's response?
Repository:
rC Clang
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D55029/new/
https://reviews.llvm.org/D55029
ruiu added a comment.
Yeah, I believe this patch is fine to submit, but since Peter is in a different
timezone, I wanted give him a chance to review this one.
Repository:
rC Clang
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D55029/new/
https://reviews.llvm.org/D55029
ruiu added a comment.
To be honest, I don't think I would lgtm a patch that changes lld's default
behavior depending on host/target only for NetBSD, and it seems like a NetBSD's
issue rather than an lld's issue. As I said, could you please make an effort to
pass the flags you need from the
ruiu added a comment.
I want to handle NetBSD in the same way as the other operating systems. I'm
sorry to have been saying no to a few recent patches for NetBSD, but I think
that's for a good reason, and I don't think you presented a convincing reason
why we had to handle only NetBSD
ruiu added a comment.
I'm sorry to say this but I don't think this is a good approach. Just like
changing the default behavior depending on host hurts cross-build and is
against the policy of the lld driver, changing the default behavior depending
on the target hurts it too. Imagine that you
1 - 100 of 156 matches
Mail list logo
