[PATCH] D54334: [AVR] Automatically link CRT and libgcc from the system avr-gcc

[Dylan McKay via Phabricator via cfe-commits]

dylanmckay added inline comments.


================
Comment at: include/clang/Basic/DiagnosticDriverKinds.td:44-45
+def warn_drv_avr_family_linking_stdlibs_not_implemented: Warning<
+  "support for linking stdlibs for microcontroller '%0' is not implemented, "
+  "please file an AVR backend bug at http://bugs.llvm.org/ with your mcu name">,
+  InGroup<AVRRtlibLinkingQuirks>;
----------------
aaron.ballman wrote:
> This is novel for diagnostics -- we don't have any other diagnostics that recommend filing bugs. This doesn't strike me as something a naive developer will just happen across by accident, so I would probably drop from the comma onward and assume the user knows they can report bugs if they really care.
I agree


================
Comment at: lib/Driver/ToolChains/AVR.cpp:40
+
+const std::string PossibleAVRLibcLocations[] = {
+  "/usr/avr",
----------------
aaron.ballman wrote:
> Why use `std::string` here when below you use it as a list of `StringRef`s?
Good point, have fixed.


================
Comment at: lib/Driver/ToolChains/AVR.cpp:162
+
+  return Optional<std::string>();
+}
----------------
aaron.ballman wrote:
> `return llvm::None;`
Didn't know about that, thanks!


================
Comment at: lib/Driver/ToolChains/AVR.h:45-47
+  Linker(const llvm::Triple &Triple,
+         const ToolChain &TC,
+         bool LinkStdlib)
----------------
aaron.ballman wrote:
> Did clang-format produce this? For some reason, it looks off to my eyes.
I have now run clang-format on the whole patch.


Repository:
  rC Clang

https://reviews.llvm.org/D54334