thanks for the response, On Wed, Oct 6, 2010 at 1:53 PM, Michael Spencer <bigcheesegs at gmail.com>wrote:> On Sun, Sep 26, 2010 at 4:02 PM, Nathan Jeffords > <blunted2night at gmail.com> wrote: > > Hi guys, > > Attached is a set of patches that adds detailed argument information to > the > > output of the MCLoggingStreamer that may be enabled when outputting an > > object file from llc or llvm-mc. It is broken into three pieces. > > The first patch allows a MCSection to report a name to be used for > > diagnostic purposes. > > I'm not sure if it may be a better idea to just make getSectionName > virtual in the base class instead of adding a new getName function > that just forwards to getSectionName in each subclass. Although > getName is more consistent with the rest of the MC api. > > I propose that we make getSectionName virtual in the base class and > then rename it to getName as it's more consistent and we already know > it is a section because of its type. >>From what I can see, getSectionName is only implemented on MachO and itdoesn't report the full name.> > > The second patch updates MCLoggingStreamer to enhance its output. > > LGTM. > > > This third patch enhances the output for instruction in > > the MCLoggingStreamer. > > I think OwningPtr would fix the problems here. Just have the logging > streamer be responsible for deleting the printer. > >As the patch stands, if the output type is assembler, the instruction pointer is created for use with the assembly streamer which I presume takes responsibility for destroying it, otherwise it get created solely for the streamer. Perhaps the cleanest approach would be to instantiate a separate instruction printer just for the logging streamer so their is no confusion as to who's responsibility it is to clean it up.> - Michael Spencer > > > The first two patches are ready to go in my opinion. But the third > doesn't > > deal with ownership of the instruction printer correctly I think, and it > > maybe too much for what it gives back. > > -Nathan > > _______________________________________________ > > LLVM Developers mailing list > > LLVMdev at cs.uiuc.edu http://llvm.cs.uiuc.edu > > http://lists.cs.uiuc.edu/mailman/listinfo/llvmdev > > > > >On another note, do you have any advise on staging multiple patches? When generating this set of patches, I had to have three separate copies of code, then using WinMerge, pull the changes for a patch into an intermediate, from their I diffed it against the previous version and generated a patch, then apply the patch to the previous version and repeated the process for each successive patch. I was looking into using git somehow to create patches for commits, does that seem reasonable? - Nathan -------------- next part -------------- An HTML attachment was scrubbed... URL: <http://lists.llvm.org/pipermail/llvm-dev/attachments/20101007/3b24d2f5/attachment.html>
On Fri, Oct 8, 2010 at 1:07 AM, Nathan Jeffords <blunted2night at gmail.com> wrote:> thanks for the response, > > On Wed, Oct 6, 2010 at 1:53 PM, Michael Spencer <bigcheesegs at gmail.com> > wrote: >> >> On Sun, Sep 26, 2010 at 4:02 PM, Nathan Jeffords >> <blunted2night at gmail.com> wrote: >> > Hi guys, >> > Attached is a set of patches that adds detailed argument information to >> > the >> > output of the MCLoggingStreamer that may be enabled when outputting an >> > object file from llc or llvm-mc. It is broken into three pieces. >> > The first patch allows a MCSection to report a name to be used for >> > diagnostic purposes. >> >> I'm not sure if it may be a better idea to just make getSectionName >> virtual in the base class instead of adding a new getName function >> that just forwards to getSectionName in each subclass. Although >> getName is more consistent with the rest of the MC api. >> >> I propose that we make getSectionName virtual in the base class and >> then rename it to getName as it's more consistent and we already know >> it is a section because of its type. > > From what I can see, getSectionName is only implemented on MachO and it > doesn't report the full name.I see it on all 3. Although on MachO it seems it is different.>> >> > The second patch updates MCLoggingStreamer to enhance its output. >> >> LGTM. >> >> > This third patch enhances the output for instruction in >> > the MCLoggingStreamer. >> >> I think OwningPtr would fix the problems here. Just have the logging >> streamer be responsible for deleting the printer. >> > > As the patch stands, if the output type is assembler, the instruction > pointer is created for use with the assembly streamer which I presume takes > responsibility for destroying it, otherwise it get created solely for the > streamer. Perhaps the cleanest approach would be to instantiate > a separate instruction printer just for the logging streamer so their is no > confusion as to who's responsibility it is to clean it up.Ah, it seems I completely ignored that InstPrinter was used other places. The logging streamer does indeed need its own copy.>> >> - Michael Spencer >> >> > The first two patches are ready to go in my opinion. But the third >> > doesn't >> > deal with ownership of the instruction printer correctly I think, and it >> > maybe too much for what it gives back. >> > -Nathan >> > _______________________________________________ >> > LLVM Developers mailing list >> > LLVMdev at cs.uiuc.edu http://llvm.cs.uiuc.edu >> > http://lists.cs.uiuc.edu/mailman/listinfo/llvmdev >> > >> > > > On another note, do you have any advise on staging multiple patches? When > generating this set of patches, I had to have three separate copies of code, > then using WinMerge, pull the changes for a patch into an intermediate, > from their I diffed it against the previous version and generated a patch, > then apply the patch to the previous version and repeated the process for > each successive patch. > I was looking into using git somehow to create patches for commits, does > that seem reasonable? > - NathanI use git exclusively. It makes keeping track of separate patch sets trivial. Just make a branch for each patch set, and have as many commits in each branch as you desire. To upstream them just do "git svn rebase". You can also merge in other branches. To export patches you can just use format-patch which will make a .patch file for each commit in the branch, although I think you have to remove the headers to get coreutils-patch to apply it. - Michael Spencer
> > > I use git exclusively. It makes keeping track of separate patch sets > trivial. Just make a branch for each patch set, and have as many > commits in each branch as you desire. To upstream them just do "git > svn rebase". You can also merge in other branches. To export patches > you can just use format-patch which will make a .patch file for each > commit in the branch, although I think you have to remove the headers > to get coreutils-patch to apply it. > > - Michael Spencer >Is their any problem when a patch is committed, then gets pulled back into your git fork? Does it treat the changes coming from the svn mirror as conflicts? -------------- next part -------------- An HTML attachment was scrubbed... URL: <http://lists.llvm.org/pipermail/llvm-dev/attachments/20101008/48b04c99/attachment.html>