[PATCH] D20331: [llvm-mc] - Teach llvm-mc to generate compressed debug sections in zlib style.

[George Rimar via llvm-commits]

grimar added inline comments.

================
Comment at: lib/MC/ELFObjectWriter.cpp:576
@@ -575,4 +575,3 @@
   auto &Sec = cast<MCSectionELF>(Sym->getSection());
-  unsigned Flags = Sec.getFlags();
-  if (Flags & ELF::SHF_MERGE) {
+  if (Sec.Flags & ELF::SHF_MERGE) {
     if (C != 0)
----------------
grimar wrote:
> davide wrote:
> > davide wrote:
> > > grimar wrote:
> > > > davide wrote:
> > > > > You can change that separately, if you really like. It's unrelated, and makes the patch slightly less readable.
> > > > Please see my comment below.
> > > I can review them at the same time but they should be committed separately.
> > I saw your comment. There's no need to change at the same time the access control and remove the getter. Leave it private, and leave the getter to keep the patch small.  You can do that in a separate commit.
> Sorry but that means I need to intoduce setter to change Flags field ?
> ```
> // Set the compressed flag. That is zlib style.
>   Section.Flags |= ELF::SHF_COMPRESSED;
> 
> ```
> So do I correctly understand that you want me to create setter for this patch and remove it and getter in following commit ? what is the point for that ? May be let me just make Flags public at first ?
Returned back original code, added setter for Flags instead.

================
Comment at: lib/MC/MCSectionELF.cpp:162
@@ -161,5 +161,3 @@
 
-bool MCSectionELF::UseCodeAlign() const {
-  return getFlags() & ELF::SHF_EXECINSTR;
-}
+bool MCSectionELF::UseCodeAlign() const { return Flags & ELF::SHF_EXECINSTR; }
 
----------------
rafael wrote:
> grimar wrote:
> > davide wrote:
> > > Unrelated.
> > I am sorry but I find this and above change directly related since that changes are connected to what this patch do.
> > But if you think i need to make Flags variable public at first in separate commit, then I have no problems with that, I just think if this whole patch be approved, then I`ll do 2 commits, but otherwise there is probably no need to perform changes with Flags member.
> Lets please do this first with just a getter and setter to make the patch easy to read.
Done.


http://reviews.llvm.org/D20331