This is an archive of the discontinued LLVM Phabricator instance.

[AVR] Add AVRTargetStreamers
ClosedPublic

Authored by dylanmckay on Jan 19 2016, 2:32 AM.

Details

Summary

Add AVR target streamer classes. A fairly trivial patch.

Diff Detail

Event Timeline

dylanmckay updated this revision to Diff 45236.Jan 19 2016, 2:32 AM
dylanmckay retitled this revision from to [AVR] Add AVRTargetStreamers.
dylanmckay updated this object.
dylanmckay added reviewers: arsenm, hfinkel, dsanders.
dylanmckay edited edge metadata.Jan 19 2016, 2:35 AM
dylanmckay added a subscriber: llvm-commits.
arsenm added inline comments.Jan 19 2016, 10:41 AM
lib/Target/AVR/MCTargetDesc/AVRTargetStreamer.cpp
36–37 ↗(On Diff #45236)

This should probably be a function rather than just marking a region with braces

dylanmckay updated this revision to Diff 46502.Jan 31 2016, 5:40 PM

Move EFlag calculation into separate function

dylanmckay marked an inline comment as done.Jan 31 2016, 5:40 PM
dylanmckay added inline comments.
lib/Target/AVR/MCTargetDesc/AVRTargetStreamer.cpp
37–38 ↗(On Diff #46502)

Done - cheers Matt.

dylanmckay marked 2 inline comments as done.Jan 31 2016, 5:41 PM
arsenm edited edge metadata.Feb 11 2016, 6:59 PM

I'm not particularly familiar with MC bits, but besides these minor cleanups this looks OK to me

lib/Target/AVR/MCTargetDesc/AVRTargetStreamer.cpp
21–22 ↗(On Diff #46502)

Convention is for new functions to start with a lowercase letter, and to use static functions instead of an anonymous namespace.

74–76 ↗(On Diff #46502)

Convention is also that the ELF / other specific object format Streamer be kept in a separate file

dylanmckay updated this revision to Diff 47960.Feb 15 2016, 1:30 AM
dylanmckay marked 2 inline comments as done.
dylanmckay edited edge metadata.

Second round of code review

@arsenm fixed up your points - cheers :)

arsenm accepted this revision.May 19 2016, 10:38 AM
arsenm edited edge metadata.

LGTM with comment fixes + makefile dropped

lib/Target/AVR/MCTargetDesc/AVRELFStreamer.h
18–20

C++ style comments

lib/Target/AVR/MCTargetDesc/AVRTargetStreamer.h
17–19

Ditto

lib/Target/AVR/MCTargetDesc/Makefile
1–16 ↗(On Diff #47960)

Not necessary anymore

This revision is now accepted and ready to land.May 19 2016, 10:38 AM
dylanmckay marked 3 inline comments as done.May 19 2016, 6:14 PM
dylanmckay updated this revision to Diff 57883.May 19 2016, 6:14 PM
dylanmckay edited edge metadata.

Removed Makefile and C++ style comments

dylanmckay closed this revision.May 19 2016, 6:25 PM

Committed in r270171