Menu

Humble suggestion

Developers
a2z
2004-03-04
2004-03-17
  • a2z

    a2z - 2004-03-04

    From my POV, JCF v2 is, IMHO, has become more than good enough for its current version.

    Apart from the one parsing issue (whose poster did not back it up) there is nothing important to do as far as parsing is concerned.

    Performance-wise: There is hardly anything significant that can be done without major architectural overhaul.

    Might I suggest that you call it release and start with v3.x.

     
    • Anthony Steele

      Anthony Steele - 2004-03-04

      I agree that the parsing is solid and that major performance gains are unlikely without major rewrites.

      The one parsing issue that you mention may be "inline", which has in fact been addressed (see the comments on the bug report).

      Since there are some changes, which make a 10-20% speed improvement over beta 13, now in CVS, and I am still considering the possibility of entirely removing TRVisitResult, replacing it with a Boolean return value, I suggest just one more beta this month.

      Then a code freeze (i.e. no features and performance tuning, only bug fixes) and a 2.0 release.

      After that is a 2.1 release with potential new features, redesigns etc. Can this go in parallel during the code freeze?

       
      • a2z

        a2z - 2004-03-04

        In that case I'd suggest a code freeze for 2.0 now, except for bugs.

        WRT TRVisitResult, I'd leave it as it is. I am about to finish a class that will not disturb you or the code :-) but will replace TObjectlist. This is a polymorphic tree (i.e. something like what you did in TParseNodeTree but without the TObjectlist and its pains). Then you can decide what to do with it.

        I have just finished checking it for mem leaks etc.

        In my machine (dual opteron, 2 gig ram blah blah) it takes less than 0.4 sec to create *and* destroy 1 million items. Obviously, an item can be any level deep or anywhere in the tree.

        I will upload it for you to take a look.

        But, the reason why I suggested a v3 was because I could see a couple of infrastructural changes. Also, JCF need to, IMHO, be detached from GUI even more so. IOW, all the JCF engine should be turned into a single class. A class that receives its data in terms of a stream (TFileStream or any TStream descendant) and returns the result also as stream (user supplied).

        Any messages etc chould be event methods.

        The nice thing about doing this would be to encapsulate JCF engine in a thread. Which will not only have a very much responsive feel, but the GUI could then work on more than one file --if need be.

         
        • Anthony Steele

          Anthony Steele - 2004-03-06

          > WRT TRVisitResult, I'd leave it as it is.

          Too late, it is gone.

           
          • a2z

            a2z - 2004-03-07

            > > WRT TRVisitResult, I'd leave it as it is.
            > Too late, it is gone.

            Well, good riddance then :-)

            I never liked the blighter anyway.

             
        • Anthony Steele

          Anthony Steele - 2004-03-06

          "IOW, all the JCF engine should be turned into a single class. A class that receives its data in terms of a stream (TFileStream or any TStream descendant) and returns the result also as stream (user supplied)."

          I wouldn't turn it into a single class, but I would faade it with a single class (http://en.wikipedia.org/wiki/Facade_pattern) TConverter currently fills this role, and is subclassed to work with files or in-memory strings. I have knocked up a (completely untested) subclass that deals with TStream, and it is attached below, but if that route is chosen then it might as well do streams all the way in, and do away with TCodeReader and TCodeWriter.

          Putting a Tconverter in a thread is simple (ok, getting messages back to the gui will require calls via Synchronize() ) and this code is also attached.

          Not in V2.0

          unit StreamConverter;

          interface

          uses
            { delphi }Classes,
            { local }Converter, CodeReader, CodeWriter;

          type
            TStreamReader = class(TCodeReader)
            private
              { property implementation }
              fcInputStream: TStream;

            protected
              procedure ReadFromSource; override;
            public
              procedure Clear; override;

              property InputStream: TStream Read fcInputStream Write fcInputStream;
            end;

            TStreamWriter = class(TCodeWriter)
            private
              { properties }
              fcOutputStream: TStream;

            protected

            public
              constructor Create; override;
              procedure Close; override;

              property OutputStream: TStream Read fcOutputStream Write fcOutputStream;
            end;

            TStreamConverter = class(TConverter)
            private
              fcMessageStream: TStream;

              procedure SetInputStream(const pcStream: TStream);
              procedure SetOutputStream(const pcStream: TStream);
              procedure SetMessageStream(const pcStream: TStream);

              function GetInputStream: TStream;
              function GetOutputStream: TStream;
              function GetMessageStream: TStream;

            protected
              function CreateReader: TCodeReader; override;
              function CreateWriter: TCodeWriter; override;
              function OriginalFileName: string; override;

              procedure SendStatusMessage(const psFile, psMessage: string;
                const piY, piX: integer); override;

            public
              constructor Create;

              procedure Convert; override;

              property InputStream: TStream Read GetInputStream Write SetInputStream;
              property OutputStream: TStream Read GetOutputStream Write SetOutputStream;
              property MessageStream: TStream Read GetMessageStream Write SetMessageStream;

            end;

          implementation

          uses SysUtils, JclStrings;

          procedure TStreamReader.Clear;
          begin
            inherited;
            fcInputStream := nil;
          end;

          procedure TStreamReader.ReadFromSource;
          var
            lpData: Pchar;
          begin
            if fbHasRead then
              exit;

            Assert((fcInputStream <> nil), 'No source stream');

            // read the data
            fiSourceLength := fcInputStream.Size;

            SetLength(fsSource, fiSourceLength);
            lpData := pchar(fsSource);
            fcInputStream.Read(lpData, fiSourceLength);

            fiReadIndex    := 1;
            fiBufferLength := 1;
            fbHasRead      := True;
          end;

          constructor TStreamWriter.Create;
          begin
            inherited;
            fcOutputStream := nil;
          end;

          procedure TStreamWriter.Close;
          var
            lpData: Pchar;
          begin
            if BOF then
              exit;

            Assert(fcOutputStream <> nil);

            BeforeWrite;

            lpData := pchar(fsDestText);
            fcOutputStream.Write(lpData, length(fsDestText));
          end;

          constructor TStreamConverter.Create;
          begin
            inherited;

            fcMessageStream := nil;
          end;

          procedure TStreamConverter.Convert;
          begin
            // show message on popup if there is no message output
            GuiMessages := (fcMessageStream = nil);

            DoConvertUnit;
          end;

          function TStreamConverter.CreateReader: TCodeReader;
          begin
            Result := TStreamReader.Create;
          end;

          function TStreamConverter.CreateWriter: TCodeWriter;
          begin
            Result := TStreamWriter.Create;
          end;

          function TStreamConverter.GetMessageStream: TStream;
          begin
            Result := fcMessageStream;
          end;

          function TStreamConverter.GetInputStream: TStream;
          begin
            Result := (fcReader as TStreamReader).InputStream;
          end;

          function TStreamConverter.GetOutputStream: TStream;
          begin
            Result := (fcWriter as TStreamWriter).OutputStream;
          end;

          function TStreamConverter.OriginalFileName: string;
          begin
            Result := 'text';
          end;

          procedure TStreamConverter.SetMessageStream(const pcStream: TStream);
          begin
            fcMessageStream := pcStream;
          end;

          procedure TStreamConverter.SetInputStream(const pcStream: TStream);
          begin
            (fcReader as TStreamReader).InputStream := pcStream;
          end;

          procedure TStreamConverter.SetOutputStream(const pcStream: TStream);
          begin
            (fcWriter as TStreamWriter).OutputStream := pcStream;
          end;

          procedure TStreamConverter.SendStatusMessage(const psFile, psMessage: string;
            const piY, piX: integer);
          var
            lsWholeMessage: string;
            lpData: Pchar;
          begin
            if fcMessageStream <> nil then
            begin
              lsWholeMessage := psMessage;
              if (piY >= 0) and (piX >= 0) then
                lsWholeMessage := lsWholeMessage + ' at line ' + IntToStr(piY) +
                  ' col ' + IntToStr(piX) + AnsiLineBreak;

              lpData := pchar(lsWholeMessage);
              fcMessageStream.Write(lpData, length(lsWholeMessage));
            end;

          end;

          type
            TConvertThread = class(TThread)
            private
              fcConverter: TConverter;
            public
              procedure Execute; override;

              constructor Create(const pbCreateSuspended: Boolean;
                const pcCOnverter: TConverter);
            end;

          constructor TConvertThread.Create(const pbCreateSuspended: Boolean;
                const pcConverter: TConverter);
          begin
            inherited Create(pbCreateSuspended);

            Assert(pcConverter <> nil);
            fcConverter := pcConverter;
          end;

          procedure TConvertThread.Execute;
          begin
            fcConverter.Convert;
          end;

          end.

           
          • a2z

            a2z - 2004-03-07

            > I wouldn't turn it into a single class, but I would faade it with a
            > single class (http://en.wikipedia.org/wiki/Facade_pattern)

            I, personally, do not see what magic a new metaphor will bring.

            From where I look at it, the whole thing boils down to these top-level
            classes: (all names are ficticious)

            TCodeFormatter: This is the actual engine. It does all the formatting.

            TFormatSettings: This supplies various user defined formatting settings
            to TCodeFormatter.

            I dont see what else is needed and why. Please discuss.

            > TConverter currently fills this role, and is subclassed to work with
            > files or in-memory strings. I have knocked up a (completely untested)
            > subclass
            [snip]
            > but if that route is chosen then it might as well do streams all the
            > way in, and do away with TCodeReader and TCodeWriter.

            Again, a core class wrapped in a superfluous class...

            Why dont you simply let TCodeFormatter (using the ficticious name above,
            for the sake of arguement) simply have 2 properties:

            ___Property InputStream: TStream Read FInputStream Write SetInputStream;
            ___Property OutputStream: TStream Read FOutputStream Write SetOutputStream;

            In general, I can not think of any situation where it would be wiser to
            use anything other than TStream. True, especially, in the case of a code
            formatter where you do not alter the source stream --you are not supposed
            to, anyway.

            > Putting a Tconverter in a thread is simple (ok, getting messages back
            > to the gui will require calls via Synchronize() ) and this code is
            > also attached.

            I dont like the MessageStream idea at all. I would like to receive any
            messages in real-time. Not after the fact. So, IMO, messages should be
            event methods --the more varied and rich they are the better. IOW, do
            not simply provide an OnError method; make it rich enough. Such as OnHint,
            OnWarning, OnError, OnFatalError etc..

            WRT threading: No, I did not mean to say that TCodeFormatter should be
            threaded. As long as you dont use any global variables, it will be
            thread-ready and that is good enough. Should anyone wish to use it in
            a multi-threaded fashion, they can easily encapsulate TCodeFormatter
            in a thread with all the Synchronize() issues taken care of then.

             
    • Anthony Steele

      Anthony Steele - 2004-03-07

      I, personally, do not see what magic a new metaphor will bring.
      I dont see what else is needed and why. Please discuss.

      The code formatter is not a single class, it is a whole bunch of processor classes descended from TBaseTreeNodeVisitor, along with classes that support them. This is a very good thing - it is much simpler than it would otherwise be, and anyone writing a new processor can write a class that does one code transformation and nothing else, without worrying much about context and interference and other complex code in the linebreaker or whatever.

      It's not a new metaphor, it's keeping and strengthening the existing metaphor, that there's a simple front end to all of this. I take your point that it could be even simpler to the person using the formatter engine, not extending it.

       
      • a2z

        a2z - 2004-03-10

        > The code formatter is not a single class, it is a whole
        > bunch of processor classes descended from
        > TBaseTreeNodeVisitor, along with classes that
        > support them. This is a very good thing

        This is the bit that I fail to agree. I dont see how it could be a good thing to sort out from a myriad of classes that are doing slightly different things.

        Why not one class with distinctly-named procedures representing the functionality of each class?

        Just look at the pros: Faster. More memory efficient (you dont create and destroy so many objects). It is as extesible if not more. Just add the required code to a derivative class.

        -----

        And, about the facade thing: I wasn't talking abaout the user; the user does not get to know about it. But, IMO, it is a new metaphor. A new class kind etc.

        Already, when formatting MSHTML_TLB.pas, JCF hits 120 MB RAM.. OK, RAM is cheap, but watching it I stand in amazement and wonder what exactly is taking all that space...

        JCF uses more memory than D7..

        You have to keep in mind that allocating and deallocating (creating and destroying objects) is not exactly free in terms of CPU time.

        I have tried RecyclerMM and a commercial one, to see if they help in reducing the load. They did not help much. IMO, 120 MB RAM usage is excessive and highlights an underlying design issue. my 0.02 cu

         
        • Anthony Steele

          Anthony Steele - 2004-03-10

          > This is the bit that I fail to agree.
          > I dont see how it could be a good thing to sort
          > out from a myriad of classes that are doing
          > slightly different things.

          > Why not one class with distinctly-named procedures
          > representing the functionality of each class?

          Encapsulation, being able to understand each class in isolation, is a good thing. It makes the source processing very modular and extensible.

           
          • a2z

            a2z - 2004-03-11

            > Encapsulation,

            I love these big words :-)

            > being able to understand each class in isolation, is a good thing.
            > It makes the source processing very modular and extensible. 

            Take a look at the blob below. It contains all the stuff
            of TBaseTreeNodeVisitor and whatever derived from it. Comments at the end of each line show where that line belongs to.

            If you simply rename a few of those overridden calls, you'd end up with a simple class that people would understand better --IMO.

            'Encapsulation, 'Polymorphism' etc. will be useful when someone (other than you) wants to extend that class.

              TTreeNodeVisitor = class(TObject)
              private
                fbEnabled: boolean; {TSwitchableVisitor}
                fbHasPostVisit: Boolean; {TBaseTreeNodeVisitor}
                fbHasPreVisit: Boolean; {TBaseTreeNodeVisitor}
                fbHasSourceTokenVisit: Boolean; {TBaseTreeNodeVisitor}
                fbWorkIsDone: boolean; {TMozComment}
                fcIndentNodes: TObjectList; {TVisitSetNestings}
                fcRunningTotals: TNestingLevelList; {TVisitSetNestings}
                feFormatFlags: TFormatFlags; {TSwitchableVisitor}
                fiAllProcs: integer; {TBasicStats}
                fiClasses: integer; {TBasicStats}
                fiCommentChars: integer; {TBasicStats}
                fiCommentTokens: integer; {TBasicStats}
                fiConsts: integer; {TBasicStats}
                fiInterfaceProcs: integer; {TBasicStats}
                fiInterfaces: integer; {TBasicStats}
                fiLines: integer; {TBasicStats}
                fiSolidChars: integer; {TBasicStats}
                fiSolidTokenOnLineIndex: integer; {TVisitSetXY}
                fiSolidTokens: integer; {TBasicStats}
                fiSpaceChars: integer; {TBasicStats}
                fiSpaceTokens: integer; {TBasicStats}
                fiTotalChars: integer; {TBasicStats}
                fiTotalTokens: integer; {TBasicStats}
                fiTypes: integer; {TBasicStats}
                fiX: integer; {TVisitSetXY}
                fiY: integer; {TVisitSetXY}
                liGlobalVars: integer; {TBasicStats}
                liInterfaceGlobalVars: integer; {TBasicStats}

                procedure ProcessNode(const pcNode: TObject; const pbIncrement: boolean); {TVisitSetNestings}
              public
                constructor Create; virtual; {TBaseTreeNodeVisitor}
                destructor Destroy; override; {TBaseTreeNodeVisitor}

                function FinalSummary(var psMessage: string): boolean; override; {TBaseTreeNodeVisitor}
                function FinalSummary(var psMessage: string): boolean; override; {TBasicStats}
                function FinalSummary(var psMessage: string): boolean; virtual; {TBaseTreeNodeVisitor}

                function IsIncludedInSettings: boolean; override; {TBasicStats}
                function IsIncludedInSettings: boolean; override; {TMozComment}
                function IsIncludedInSettings: boolean; virtual; {TBaseTreeNodeVisitor}

                procedure PostVisitParseTreeNode(const pcNode: TObject); override; {TVisitSetNestings}
                procedure PostVisitParseTreeNode(const pcNode: TObject); virtual; {TBaseTreeNodeVisitor}

                procedure PreVisitParseTreeNode(const pcNode: TObject; var prVisitResult: TRVisitResult); override; {TBasicStats}
                procedure PreVisitParseTreeNode(const pcNode: TObject; var prVisitResult: TRVisitResult); override; {TVisitSetNestings}
                procedure PreVisitParseTreeNode(const pcNode: TObject; var prVisitResult: TRVisitResult); virtual; {TBaseTreeNodeVisitor}

                procedure VisitSourceToken(const pcNode: TObject; var prVisitResult: TRVisitResult); override; {TBasicStats}
                procedure VisitSourceToken(const pcToken: TObject; var prVisitResult: TRVisitResult); override; {TMozComment}
                procedure VisitSourceToken(const pcToken: TObject; var prVisitResult: TRVisitResult); override; {TSwitchableVisitor}
                procedure VisitSourceToken(const pcToken: TObject; var prVisitResult: TRVisitResult); override; {TVisitSetNestings}
                procedure VisitSourceToken(const pcToken: TObject; var prVisitResult: TRVisitResult); override; {TVisitSetXY}
                procedure VisitSourceToken(const pcToken: TObject; var prVisitResult: TRVisitResult); override; {TVisitStripEmptySpace}
                procedure VisitSourceToken(const pcToken: TObject; var prVisitResult: TRVisitResult); virtual; {TBaseTreeNodeVisitor}

                property FormatFlags: TFormatFlags Read feFormatFlags Write feFormatFlags; {TSwitchableVisitor}
                property HasPostVisit: boolean read fbHasPostVisit write fbHasPostVisit; {TBaseTreeNodeVisitor}
                property HasPreVisit: boolean read fbHasPreVisit write fbHasPreVisit; {TBaseTreeNodeVisitor}
                property HasSourceTokenVisit: boolean read fbHasSourceTokenVisit write fbHasSourceTokenVisit; {TBaseTreeNodeVisitor}
              end;

             
    • Anthony Steele

      Anthony Steele - 2004-03-17

      >'Encapsulation, 'Polymorphism' etc. will be useful when someone (other than you) wants to extend that class.

      Indeed, the idea is that it will be easy to add a new process to the formatter, by coding one or more simple formatting processes, transformation or stats-gathering or source code in a class.

      It's a much simpler API than the code you've been reading through, and I hope that more people (by that I mean *any* people) pick it up.

      There's a lot that could be done, which I don't have the time or pressing need to do. For instance, it should be straightforward to do things such as making sure each if...then has a begin...end block, or conversely removing begin...end blocks that contain only one statement if that's what suits you.

      For instance, I coded BasicStats.pas in a few hours just to show the simplest numbers that could be extracted from reading the code.

      Anthony

       

Log in to post a comment.

Want the latest updates on software, tech news, and AI?
Get latest updates about software, tech news, and AI from SourceForge directly in your inbox once a month.