[PATCH] D36209: [Bash-autocompletion] Add comment to test so that it is easier to fix

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 ___

[PATCH] D36209: [Bash-autocompletion] Add comment to test so that it is easier to fix

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

[PATCH] D36567: [Bash-autocompletion] Add --autocomplete flag to 5.0 release notes

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 ___ cfe-com

[PATCH] D36567: [Bash-autocompletion] Add --autocomplete flag to 5.0 release notes

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

[PATCH] D36782: [Bash-autocompletion] Add support for static analyzer flags

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 mechani

[PATCH] D36820: [Bash-autocompletion] Add support for -std=

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

[PATCH] D36782: [Bash-autocompletion] Add support for static analyzer flags

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: llvm/utils/TableGen/OptParserEmit

[PATCH] D36820: [Bash-autocompletion] Add support for -std=

ruiu added inline comments. Comment at: llvm/utils/TableGen/OptParserEmitter.cpp:314 for (const std::string &Pref : R.getValueAsListOfStrings("Prefixes")) { -OS << "bool ValuesWereAdded = "; +OS << "ValuesWereAdded = "; OS << "Opt.addValues(";

[PATCH] D36820: [Bash-autocompletion] Add support for -std=

ruiu added inline comments. Comment at: llvm/utils/TableGen/OptParserEmitter.cpp:308 const Record &R = *Opts[I]; if (!isa(R.getValueInit("ValuesCode"))) { OS << "{\n"; You can flip the condition to do early continue. Comment at

[PATCH] D36820: [Bash-autocompletion] Add support for -std=

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 http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commi

[PATCH] D125624: [gold] Remove an external dependency to GNU binutils' header file

ruiu added inline comments. Herald added a subscriber: Enna1. Comment at: llvm/tools/gold/gold-plugin.cpp:44-52 // FIXME: remove this declaration when we stop maintaining Ubuntu Quantal and // Precise and Debian Wheezy (binutils 2.23 is required) #define LDPO_PIE 3 #define

[PATCH] D125624: [gold] Remove an external dependency to GNU binutils' header file

ruiu added a comment. Does it? What is your test command? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D125624/new/ https://reviews.llvm.org/D125624 ___ cfe-commits mailing list cfe-commits@lists.llvm.or

[PATCH] D125624: [gold] Remove an external dependency to GNU binutils' header file

ruiu added a comment. I'm not sure if my credential is still valid. Do you mind if I ask you to submit this for me? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D125624/new/ https://reviews.llvm.org/D125624 ___

[PATCH] D125624: [gold] Remove an external dependency to GNU binutils' header file

ruiu added a comment. The motivation of doing this is to be able to build LLVMgold.so without binutils' source files and make it clear that LLVMgold.so does not include any GPL code. The header file defines the public interface between linker plugins and compilers (and other tools such as `ar`

[PATCH] D125624: [gold] Remove an external dependency to GNU binutils' header file

ruiu added a comment. > So what happens if LLVMgold.so uses one of the new structs or constants and > then is built and run on system where binutils is old enough to not have > these new structs and constants? The same thing can happen with GCC and binutils. Imagine that you install a newer

[PATCH] D125624: [gold] Remove an external dependency to GNU binutils' header file

ruiu added a comment. In D125624#3552284 , @khchen wrote: > IMO, maybe we could keep the DLLVM_BINUTILS_INCDIR option support but default > is using the Plugin.h? We can, but I wonder what is the motivation of doing it. Repository: rG LLVM Github Mo

[PATCH] D125624: [gold] Remove an external dependency to GNU binutils' header file

ruiu added a comment. > OK, as I mentioned. I think we need an attorney to review this change and > confirm that it actually accomplishes this goal. Can you add an attorney as a reviewer of this change so that we can proceed? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION h

[PATCH] D70702: Use InitLLVM to setup a pretty stack printer

ruiu created this revision. ruiu added a reviewer: MaskRay. Herald added subscribers: cfe-commits, usaxena95, kadircet, arphaman, jkorous. Herald added projects: clang, LLVM. InitLLVM does not only save a few lines from main() but also makes the commands do the right thing for multibyte character

[PATCH] D70702: Use InitLLVM to setup a pretty stack printer

This revision was automatically updated to reflect the committed changes. Closed by commit rG3f76260dc067: Use InitLLVM to setup a pretty stack printer (authored by ruiu). Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D70702/new/ https://reviews.llvm

[PATCH] D70694: Use InitLLVM in clang-tidy

This revision was automatically updated to reflect the committed changes. Closed by commit rGa7acba29c19a: Use InitLLVM in clang-tidy (authored by ruiu). Herald added a project: clang. Herald added a subscriber: cfe-commits. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https:

[PATCH] D70919: [Hexagon] Avoid passing unsupported options to lld when -fuse-ld=lld is used

ruiu added a comment. In D70919#1771198 , @sidneym wrote: > Remove quotes around check-not. > > -fuse-ld=lld is the correct usage. -fuse-ld=ld.lld results in an error > message: > error: invalid linker name in argument '-fuse-ld=ld.lld' `-fuse-ld` acc

[PATCH] D70048: [LLD] Add NetBSD support as a new flavor of LLD (nb.lld)

ruiu added a comment. lld is not that Linux-centric as you implied in your comment. We have code for FreeBSD, OpenBSD, AMDGPU and Fuchsia (which isn't even a POSIX system), and adding code for NetBSD is OK and is appreciated. However, the thing we've been trying to avoid is to hard-code a platf

[PATCH] D39918: [libunwind] Remove a FIXME about truncated section names

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 http://lists.llvm.org/cgi-bin/mailman/l

[PATCH] D39918: [libunwind] Remove a FIXME about truncated section names

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 ___

[PATCH] D40234: [AutoComplete] Stable sort autocomplete candidates to remove non-deterministic ordering

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. https:/

[PATCH] D40234: [AutoComplete] Use stronger sort predicate for autocomplete candidates to remove non-deterministic ordering

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 http://lists.l

[PATCH] D40234: [AutoComplete] Use stronger sort predicate for autocomplete candidates to remove non-deterministic ordering

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.

[PATCH] D31591: Fix a bug which access nullptr and cause segmentation fault

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 it'

[PATCH] D34329: [GSoC] Clang AST diffing

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, yo

[PATCH] D34706: [COFF, ARM64] Add support for Windows ARM64 COFF format

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. htt

[PATCH] D34706: [COFF, ARM64] Add support for Windows ARM64 COFF format

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 cfe-commits@lists

[PATCH] D34761: [Bash-autocompletion] Invoke clang where user called

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

[PATCH] D34924: [Bash-autocompletion] Add support for older bash version.

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 + i

[PATCH] D34924: [Bash-autocompletion] Add support for older bash version.

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 `comp

[PATCH] D34924: [Bash-autocompletion] Add support for older bash version.

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 proba

[PATCH] D34924: [Bash-autocompletion] Add support for older bash version.

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 http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commi

[PATCH] D34924: [Bash-autocompletion] Add support for older bash version.

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]}" --

[PATCH] D34927: [Bash-autocompletion] Fix a bug that -foo=bar doesn't handled properly

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 _

[PATCH] D35159: [libcxxabi][demangler] Use an AST to represent the demangled name

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 cus

[PATCH] D35448: [Bash-autocompletion] Fixed a bug on bash

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 http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commi

[PATCH] D35447: [Bash-autocompletion] Add support for -W and -Wno

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: clang/lib/Basic/Di

[PATCH] D35447: [Bash-autocompletion] Add support for -W and -Wno

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

[PATCH] D35759: [Bash-autocompletion] Show HelpText with possible flags

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 t

[PATCH] D35759: [Bash-autocompletion] Show HelpText with possible flags

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 __

[PATCH] D35759: [Bash-autocompletion] Show HelpText with possible flags

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 do

[PATCH] D35759: [Bash-autocompletion] Show HelpText with possible flags

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

[PATCH] D35763: [Bash-completion] Fixed a bug that file doesn't autocompleted after =

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 http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commi

[PATCH] D42758: Support `#pragma comment(lib, "name")` in the frontend for ELF

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:

[PATCH] D42758: Support `#pragma comment(lib, "name")` in the frontend for ELF

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 __

[PATCH] D27603: Propagate -fdiagnostics-color to LLD.

ruiu added inline comments. Comment at: lib/Driver/Tools.cpp:234 const ArgList &Args, ArgStringList &CmdArgs, const JobAction &JA) { const Driver &D = TC.getDriver(); hans wrote: > Yes, this doesn't see

[PATCH] D27603: Propagate -fdiagnostics-color to LLD.

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 `-fuse-ld=$BINDI

[PATCH] D27603: Propagate -fdiagnostics-color to LLD.

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 ___

[PATCH] D32565: Make test corrections necessary due to changing llvm::HashString

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 http://lists.llvm.org/c

[PATCH] D32646: Fix a bug that -Wmissing-braces fires on system headers

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 li

[PATCH] D32646: Fix a bug that -Wmissing-braces fires on system headers

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: lib/Sema/SemaInit.cpp:890-891

[PATCH] D32646: Fix a bug that -Wmissing-braces fires on system headers

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 cod

[PATCH] D33259: Don't defer to the GCC driver for linking arm-baremetal

ruiu added inline comments. Comment at: lib/Driver/ToolChains/BareMetal.h:42 + + const char *getDefaultLinker() const override { return "ld.lld"; } + jroelofs wrote: > compnerd wrote: > > jroelofs wrote: > > > compnerd wrote: > > > > I think that this really sho

<    1   2