r363009 - [Frontend] SetUpDiagnosticLog should handle unowned diagnostic consumer

[Ilya Biryukov via cfe-commits]

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/7117fd25/attachment.html>