r363009 - [Frontend] SetUpDiagnosticLog should handle unowned diagnostic consumer

[Alex L via cfe-commits]

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
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20190611/5f0cf8dd/attachment.html>