[Diag] Print the diagnostic ID name from localization #34319

HassanElDesouky · 2020-10-15T05:27:40Z

cc @xedin, @owenv

lib/AST/DiagnosticEngine.cpp

lib/Localization/LocalizationFormat.cpp

test/diagnostics/Localization/en_localization_parser_lookup.swift

lib/Localization/LocalizationFormat.cpp

include/swift/Localization/LocalizationFormat.h

… IDs

…calizationProducer

HassanElDesouky

I've been away from the project and C++ for quite some time now, so hopefully it's okay and I didn't made the problem worse with the allocator 😅

cc @xedin, @owenv

lib/Localization/LocalizationFormat.cpp

include/swift/Localization/LocalizationFormat.h

test/diagnostics/Localization/en_localization.swift

owenv

I don't entirely understand why it's important for the llvm::StringSaver to be an optional argument to the Localization producer constructors, but the rest of this change looks good to me for the most part 👍

include/swift/AST/DiagnosticEngine.h

owenv · 2021-03-20T22:13:50Z

@swift-ci please smoke test

HassanElDesouky · 2021-03-21T06:43:41Z

I don't entirely understand why it's important for the llvm::StringSaver to be an optional argument to the Localization producer constructors.

My motive behind this was because localization producers might be used in a other places where it doesn't make sense to use either the StringSaver or printDiagnosticName flag. I.e. you can check the swift-serialize-diagnostics.cpp that's why I wanted to make both StringSaver and printDiagnosticName have default values. Do you think that make sense?

owenv · 2021-03-21T17:19:02Z

My motive behind this was because localization producers might be used in a other places where it doesn't make sense to use either the StringSaver or printDiagnosticName flag. I.e. you can check the swift-serialize-diagnostics.cpp that's why I wanted to make both StringSaver and printDiagnosticName have default values. Do you think that make sense?

Oh, I see, thanks for the explanation! I think that's reasonable, but could you add an assert that if printDiagnosticNames is true, the llvm::StringSaver is also present? I think that will make it a little easier to understand how the different pieces of this fit together

HassanElDesouky · 2021-03-21T18:16:26Z

but could you add an assert that if printDiagnosticNames is true, the llvm::StringSaver is also present? I think that will make it a little easier to understand how the different pieces of this fit together

Yes of course! Going to add it now.

…r is also present

lib/Localization/LocalizationFormat.cpp

owenv · 2021-03-21T21:13:04Z

@swift-ci please smoke test

HassanElDesouky · 2021-03-21T22:22:44Z

@owenv I think the failure in macOS isn't related to my changes.

test/diagnostics/Localization/en_localization.swift

include/swift/AST/DiagnosticEngine.h

…alization producer

xedin

Looks much better now, still needs a bit more work to get this across the finish line.

include/swift/Localization/LocalizationFormat.h

lib/Localization/LocalizationFormat.cpp

include/swift/Localization/LocalizationFormat.h

include/swift/AST/DiagnosticEngine.h

xedin

LGTM!

xedin · 2021-03-23T20:52:14Z

test/diagnostics/Localization/fr_debug_diagnostic_name.swift

+// RUN: swift-serialize-diagnostics --input-file-path=%S/Inputs/en.yaml --output-directory=%t/ 2>&1 | %FileCheck %s
+// RUN: not %target-swift-frontend -debug-diagnostic-names -localization-path %S/Inputs -locale fr -typecheck %s 2>&1 | %FileCheck %s --check-prefix=CHECK_NAMES
+
+// CHECK: These diagnostic IDs are no longer availiable: 'not_available_in_def, not_available_in_def_2, not_available_in_def_3, not_available_in_def_4, not_available_in_def_5'


Could you please remove these diagnostics from yaml in a separate PR?

Of course! But can you explain why?

I mean shouldn't we be testing/checking on yaml.unknownIDs?

Since .def file is still a source of truth and if it doesn't have it the that diagnostic is no longer viable.

Okay... makes sense thank you!

xedin · 2021-03-23T20:53:11Z

@swift-ci please smoke test

HassanElDesouky · 2021-03-23T22:48:50Z

@xedin I'm not sure why CI is failing on macOS. I tried to take a look at the console output, however I can't quite understand it.

xedin · 2021-03-23T22:52:24Z

Don't worry, it's a driver issue people are already looking into.

xedin · 2021-03-24T03:30:30Z

@swift-ci please smoke test macOS platform

xedin · 2021-03-24T05:02:54Z

@swift-ci please smoke test macOS platform

xedin self-requested a review October 15, 2020 18:46

xedin requested changes Oct 15, 2020

View reviewed changes

lib/AST/DiagnosticEngine.cpp Outdated Show resolved Hide resolved

HassanElDesouky commented Oct 21, 2020

View reviewed changes

lib/Localization/LocalizationFormat.cpp Show resolved Hide resolved

HassanElDesouky commented Oct 21, 2020

View reviewed changes

test/diagnostics/Localization/en_localization_parser_lookup.swift Outdated Show resolved Hide resolved

HassanElDesouky requested a review from xedin October 21, 2020 00:09

owenv reviewed Oct 21, 2020

View reviewed changes

lib/Localization/LocalizationFormat.cpp Outdated Show resolved Hide resolved

lib/Localization/LocalizationFormat.cpp Outdated Show resolved Hide resolved

HassanElDesouky commented Oct 25, 2020

View reviewed changes

include/swift/Localization/LocalizationFormat.h Outdated Show resolved Hide resolved

HassanElDesouky added 3 commits March 14, 2021 17:37

[Localization] Print diagnostic ID name from localization

dc5a4bd

[Localization] Make localization producer handle printing diagnostics…

1b0e8f9

… IDs

[Localization] Allocate a copy of diagnostic + name strings in Lo…

651655e

…calizationProducer

HassanElDesouky force-pushed the localization-debug-diagnostics branch from 7f4cb0c to 651655e Compare March 14, 2021 15:39

[Localization] Add an allocator to to retain diagnostic strings

ef7551d

HassanElDesouky commented Mar 17, 2021

View reviewed changes

lib/Localization/LocalizationFormat.cpp Outdated Show resolved Hide resolved

include/swift/Localization/LocalizationFormat.h Outdated Show resolved Hide resolved

test/diagnostics/Localization/en_localization.swift Outdated Show resolved Hide resolved

HassanElDesouky requested a review from owenv March 17, 2021 08:44

Merge branch 'main' into localization-debug-diagnostics

f676066

owenv reviewed Mar 20, 2021

View reviewed changes

include/swift/AST/DiagnosticEngine.h Outdated Show resolved Hide resolved

[DiagEngine] Make localizationSaver not optional and update comment

b34cf95

[Localization] Assert if printDiagnosticNames is true; the StringSave…

d4ae826

…r is also present

HassanElDesouky commented Mar 21, 2021

View reviewed changes

lib/Localization/LocalizationFormat.cpp Outdated Show resolved Hide resolved

HassanElDesouky requested a review from owenv March 21, 2021 19:03

owenv approved these changes Mar 21, 2021

View reviewed changes

lib/Localization/LocalizationFormat.cpp Outdated Show resolved Hide resolved

xedin requested changes Mar 22, 2021

View reviewed changes

test/diagnostics/Localization/en_localization.swift Outdated Show resolved Hide resolved

include/swift/AST/DiagnosticEngine.h Outdated Show resolved Hide resolved

HassanElDesouky added 2 commits March 23, 2021 10:41

[Localization] Move localization allocator from DiagEngine to the loc…

8665e0c

…alization producer

[tests] Add new line to the end of the files

65da71a

HassanElDesouky requested a review from xedin March 23, 2021 08:47

xedin requested changes Mar 23, 2021

View reviewed changes

[Localization] Correct namings and improve comment

09b794d

HassanElDesouky commented Mar 23, 2021

View reviewed changes

include/swift/AST/DiagnosticEngine.h Show resolved Hide resolved

HassanElDesouky requested a review from xedin March 23, 2021 20:42

xedin approved these changes Mar 23, 2021

View reviewed changes

xedin merged commit 2eb57d2 into swiftlang:main Mar 24, 2021

HassanElDesouky deleted the localization-debug-diagnostics branch March 25, 2021 20:17

[Diag] Print the diagnostic ID name from localization #34319

[Diag] Print the diagnostic ID name from localization #34319

Uh oh!

Conversation

HassanElDesouky commented Oct 15, 2020

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

HassanElDesouky left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

owenv left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

owenv commented Mar 20, 2021

Uh oh!

HassanElDesouky commented Mar 21, 2021

Uh oh!

owenv commented Mar 21, 2021

Uh oh!

HassanElDesouky commented Mar 21, 2021

Uh oh!

Uh oh!

Uh oh!

owenv commented Mar 21, 2021

Uh oh!

HassanElDesouky commented Mar 21, 2021

Uh oh!

Uh oh!

Uh oh!

xedin left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

xedin left a comment

Choose a reason for hiding this comment

Uh oh!

xedin Mar 23, 2021

Choose a reason for hiding this comment

Uh oh!

HassanElDesouky Mar 23, 2021

Choose a reason for hiding this comment

Uh oh!

HassanElDesouky Mar 23, 2021

Choose a reason for hiding this comment

Uh oh!

xedin Mar 23, 2021

Choose a reason for hiding this comment

Uh oh!

HassanElDesouky Mar 23, 2021

Choose a reason for hiding this comment

Uh oh!

xedin commented Mar 23, 2021

Uh oh!

HassanElDesouky commented Mar 23, 2021

Uh oh!

xedin commented Mar 23, 2021

Uh oh!

xedin commented Mar 24, 2021

Uh oh!

xedin commented Mar 24, 2021

Uh oh!

Uh oh!