[cfe-commits] [PATCH] Reverted clang-check to fully supported CommandLine Library use-case: global static variables.

[Manuel Klimek]

On Fri, Aug 17, 2012 at 11:41 AM, Chandler Carruth <chandlerc at google.com> wrote:
> Er, my suggestion was to make these *static* variables, yes, even though
> they are in a header file, and then to document thoroughly that this header
> file is *not* for use within a library, but for use directly within the file
> defining main.

I think the macro version is the lesser of the two evils: we can
either make the header special, or we can require the user to put in
an additional line into their main files.

> The current patch is different from that, can you describe how? In general
> the comments weren't clear enough for me to be confident in how you expect
> this file to be used...

Just curious what about this:

 /// @code
+/// #include "clang/Tooling/CommonOptionsParser.h"
+///
+/// DEFINE_COMMON_TOOL_OPTIONS;
+///
+/// cl::extrahelp MoreHelp("\nMore help text...");
+/// cl:opt<bool> YourOwnOpt(...);
+///
 /// int main(int argc, const char **argv) {
+///   CommonOptionsParser OptsParser(argc, argv);
+///   ClangTool Tool(*OptsParser.Compilations, OptsParser.SourcePathList);
 ///   return Tool.run(newFrontendActionFactory<clang::SyntaxOnlyAction>());
 /// }
 /// @endcode

is unclear. Is it perhaps in a place you don't expect it in? Should we
rather make that a file comment?

Cheers,
/Manuel

>
>
> On Fri, Aug 17, 2012 at 2:26 AM, Manuel Klimek <klimek at google.com> wrote:
>>
>> +using namespace llvm;
>>
>> Remove from header.
>>
>> Appart from that, lgtm, minus Chandler's approval of the general idea.
>>
>> On Thu, Aug 16, 2012 at 7:17 PM, Alexander Kornienko <alexfh at google.com>
>> wrote:
>> > On Thu, Aug 16, 2012 at 7:04 PM, Manuel Klimek <klimek at google.com>
>> > wrote:
>> >>
>> >> On Thu, Aug 16, 2012 at 5:45 PM, Alexander Kornienko
>> >> <alexfh at google.com>
>> >> wrote:
>> >> > Now it's a bit uglier, but doesn't use CommandLine Library in an
>> >> > unsupported
>> >> > way.
>> >> > Chandler, please take a look if it seems better to you.
>> >>
>> >> After a short discussion off-list we came to the conclusion that
>> >> tooling::CommonOptionsParser is a better name for the class, and
>> >> especially makes the responsibilities clearer...
>> >
>> > A new patch is attached.
>> >
>> >>
>> >>
>> >> > BTW, we still have a number of alien options defined in some llvm
>> >> > libraries
>> >> > we link with. Should we try to avoid that by de-globalizing cl
>> >> > library
>> >> > or is
>> >> > someone working on its replacement/refactoring now?
>> >
>> >
>> > --
>> > Best regards,
>> > Alexander Kornienko
>> >
>
>