r363009 - [Frontend] SetUpDiagnosticLog should handle unowned diagnostic consumer

[Alex L via cfe-commits]

Thanks, that sounds good to me. I added the assignment with the new comment
in 363199.

On Wed, 12 Jun 2019 at 03:59, Ilya Biryukov <ibiryukov at google.com> wrote:

> Sure, that should work just fine. Could you also add a comment what events
> setting the log file sets in motion? It is not obvious from the test code.
>
> On Tue, Jun 11, 2019 at 6:51 PM Alex L <arphaman at gmail.com> wrote:
>
>> Hmm, the logging was meant to exercise the creation of chained diagnostic
>> consumer, so without it the test is kind of pointless. I'll undo your
>> change, but will set the file to "-" which will print the log to STDERR and
>> won't create new files. Does that sound reasonable?
>>
>> Cheers,
>> Alex
>>
>> On Tue, 11 Jun 2019 at 02:51, Ilya Biryukov <ibiryukov at google.com> wrote:
>>
>>> Hi Alex,
>>>
>>> Just wanted to let you know that I removed logging of diagnostics into a
>>> file inside the unit test in r363041 to unbreak our integrate.
>>>
>>> On Tue, Jun 11, 2019 at 1:29 AM Alex Lorenz via cfe-commits <
>>> cfe-commits at lists.llvm.org> wrote:
>>>
>>>> Author: arphaman
>>>> Date: Mon Jun 10 16:32:42 2019
>>>> New Revision: 363009
>>>>
>>>> URL: http://llvm.org/viewvc/llvm-project?rev=363009&view=rev
>>>> Log:
>>>> [Frontend] SetUpDiagnosticLog should handle unowned diagnostic consumer
>>>> in the compiler
>>>>
>>>> The function SetUpDiagnosticLog that was called from createDiagnostics
>>>> didn't
>>>> handle the case where the diagnostics engine didn't own the diagnostics
>>>> consumer.
>>>> This is a potential problem for a clang tool, in particular some of the
>>>> follow-up
>>>> patches for clang-scan-deps will need this fix.
>>>>
>>>> Differential Revision: https://reviews.llvm.org/D63101
>>>>
>>>> Modified:
>>>>     cfe/trunk/lib/Frontend/CompilerInstance.cpp
>>>>     cfe/trunk/unittests/Frontend/CompilerInstanceTest.cpp
>>>>
>>>> Modified: cfe/trunk/lib/Frontend/CompilerInstance.cpp
>>>> URL:
>>>> http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Frontend/CompilerInstance.cpp?rev=363009&r1=363008&r2=363009&view=diff
>>>>
>>>> ==============================================================================
>>>> --- cfe/trunk/lib/Frontend/CompilerInstance.cpp (original)
>>>> +++ cfe/trunk/lib/Frontend/CompilerInstance.cpp Mon Jun 10 16:32:42 2019
>>>> @@ -232,9 +232,13 @@ static void SetUpDiagnosticLog(Diagnosti
>>>>
>>>>  std::move(StreamOwner));
>>>>    if (CodeGenOpts)
>>>>      Logger->setDwarfDebugFlags(CodeGenOpts->DwarfDebugFlags);
>>>> -  assert(Diags.ownsClient());
>>>> -  Diags.setClient(
>>>> -      new ChainedDiagnosticConsumer(Diags.takeClient(),
>>>> std::move(Logger)));
>>>> +  if (Diags.ownsClient()) {
>>>> +    Diags.setClient(
>>>> +        new ChainedDiagnosticConsumer(Diags.takeClient(),
>>>> std::move(Logger)));
>>>> +  } else {
>>>> +    Diags.setClient(
>>>> +        new ChainedDiagnosticConsumer(Diags.getClient(),
>>>> std::move(Logger)));
>>>> +  }
>>>>  }
>>>>
>>>>  static void SetupSerializedDiagnostics(DiagnosticOptions *DiagOpts,
>>>>
>>>> Modified: cfe/trunk/unittests/Frontend/CompilerInstanceTest.cpp
>>>> URL:
>>>> http://llvm.org/viewvc/llvm-project/cfe/trunk/unittests/Frontend/CompilerInstanceTest.cpp?rev=363009&r1=363008&r2=363009&view=diff
>>>>
>>>> ==============================================================================
>>>> --- cfe/trunk/unittests/Frontend/CompilerInstanceTest.cpp (original)
>>>> +++ cfe/trunk/unittests/Frontend/CompilerInstanceTest.cpp Mon Jun 10
>>>> 16:32:42 2019
>>>> @@ -8,6 +8,7 @@
>>>>
>>>>  #include "clang/Frontend/CompilerInstance.h"
>>>>  #include "clang/Frontend/CompilerInvocation.h"
>>>> +#include "clang/Frontend/TextDiagnosticPrinter.h"
>>>>  #include "llvm/Support/FileSystem.h"
>>>>  #include "llvm/Support/Format.h"
>>>>  #include "llvm/Support/ToolOutputFile.h"
>>>> @@ -70,4 +71,21 @@ TEST(CompilerInstance, DefaultVFSOverlay
>>>>    ASSERT_TRUE(Instance.getFileManager().getFile("vfs-virtual.file"));
>>>>  }
>>>>
>>>> +TEST(CompilerInstance,
>>>> AllowDiagnosticLogWithUnownedDiagnosticConsumer) {
>>>> +  auto DiagOpts = new DiagnosticOptions();
>>>> +  DiagOpts->DiagnosticLogFile = "log.diags";
>>>> +
>>>> +  // Create the diagnostic engine with unowned consumer.
>>>> +  std::string DiagnosticOutput;
>>>> +  llvm::raw_string_ostream DiagnosticsOS(DiagnosticOutput);
>>>> +  auto DiagPrinter = llvm::make_unique<TextDiagnosticPrinter>(
>>>> +      DiagnosticsOS, new DiagnosticOptions());
>>>> +  CompilerInstance Instance;
>>>> +  IntrusiveRefCntPtr<DiagnosticsEngine> Diags =
>>>> Instance.createDiagnostics(
>>>> +      DiagOpts, DiagPrinter.get(), /*ShouldOwnClient=*/false);
>>>> +
>>>> +  Diags->Report(diag::err_expected) << "no crash";
>>>> +  ASSERT_EQ(DiagnosticsOS.str(), "error: expected no crash\n");
>>>> +}
>>>> +
>>>>  } // anonymous namespace
>>>>
>>>>
>>>> _______________________________________________
>>>> cfe-commits mailing list
>>>> cfe-commits at lists.llvm.org
>>>> https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
>>>>
>>>
>>>
>>> --
>>> Regards,
>>> Ilya Biryukov
>>>
>>
>
> --
> Regards,
> Ilya Biryukov
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20190612/21db607c/attachment-0001.html>