#2869 new CompareBinaryFiles

open
nobody
5
2012-12-21
2009-06-11
Matthias
No

This is a new binarycompare as discussed in forum.

Discussion

1 2 > >> (Page 1 of 2)
  • Kimmo Varis

    Kimmo Varis - 2009-06-11

    This still needs description. Just saying its been discussed somewhere without even giving a link to discussion is big no. Please try to understand we may need to refer and look to these items after few years. What is now clear is forgotten then. Always describe patches so that the description is useful even after few years. It is the whole point of using patch tracker. Yes it requires time to write good description. Yes, everybody does it.

    This absolutely needs to be a new compare engine not just function in quick compare engine. So this is not what I wanted and described.
    -

     
  • Kimmo Varis

    Kimmo Varis - 2009-06-12

    When I talk about compare engine I mean class like ByteCompare class. There is separate methods for setting options, running the compare and setting abortable interface. These are very important things to do. Aborting must be possible for large files. The binary compare engine must be able to run as independent and selectable engine, not just quick hack of some other engine.

    It really requires some work to do. And I expect even testing takes few days. This will be very important feature in compare engines and it must be done with lots of care. And it must be tested too. I've put months of work into this current compare system so I really will make sure the new engine is done right. You can expect lots of comments.

     
  • Matthias

    Matthias - 2009-06-12

    ok, I create a complete new class, will take some days to do.
    the function you asked for are will realy work also, that's clear.

     
  • Matthias

    Matthias - 2009-06-24

    related to forum https://sourceforge.net/apps/phpbb/winmerge/viewtopic.php?f=6&t=115
    this is a new patch, where I created new classes.
    A basic FileCompare witch is used as common and inherit to BinaryCompare.
    To have have two classes makes it easier to create new engines, as the common things are allready done in basic class (FileCompare).

    you need to install also
    #2805036 GuessCodepageEncoding miss binary and UTF8
    and
    #2805040 switch to new CompareBinaryFiles

     
  • Matthias

    Matthias - 2009-06-24

    create new classes

     
  • Kimmo Varis

    Kimmo Varis - 2009-06-24

    New general file compare class is basically a good idea.

    You cannot use diffutils internal structure in other compare engines:
    + file_data * m_inf; /**< Compared files data (for diffutils). */

    This would make other engines dependent on diffutils internals which must not happen. Yes, I see quick compare uses it currently but we must fix that.

    +#include "DIFF.H"
    This is a BIG warning sign that something is wrong - it is diffutils header!

    You have copied code and comments but haven't updated them to match new place. Comments in FileCompare class talk about quick compare etc.

    FileTextStats really don't belong to base class - we only get text stats from one engine.

    What is this CCompareOptions class?

    +/**
    + * @brief Compare options used with compare -method.
    + * This class has some Basic Compare specifics in addition to general compare
    + * options.
    + */
    Doesn't make any sense to me since it just seems to be binary file compare options with another name.

    Also the name is just wrong - we don't prefix class names with "C" and now that prefix only differentiates it from other class name.

    + long tmp[2]; // pointer to file postion
    long is NOT 64-bit in 32-bit Windows. You need 64-bit file positions to support large files.

    + _lseek(m_inf[i].desc, tmp[i], SEEK_SET);
    You must use lseeki64 to support large files

    + // Did we finish both files?
    + // We set the text/binary status only for fully compared files. Only
    + // then the result is reliable.
    The comment is wrong.

    + void SetAdditionalOptions(bool stopAfterFirstDiff);
    Is there a situation when we don't want to stop after first different byte? I can' t think of any. Quick compare needs to read whole file to get reliable results since it handles text files.

    + BinaryCompareOptions *m_pOptions; /**< Compare options for diffutils. */
    Update comments.

    +bool FileCompare::SetCompareOptions(const CompareOptions & options)
    +{
    + return false;
    +}
    I don't understand this?

    +/**
    + * @brief check Abortable-interface.
    You don't check interface but you check if user wants to stop the compare.

    +/**
    + * @brief A quick compare -compare method implementation class.
    + * This compare method compares files in small blocks. Code assumes block size
    + * is in range of 32-bit int-type.
    + */
    +class FileCompare
    Update comment and perhaps describe THIS class...

    + CCompareOptions *m_pOptions; /**< Compare options for diffutils. */
    Update comment...

    + file_data * m_inf; /**< Compared files data (for diffutils). */
    Update comment...

    +/**
    + * @brief Compare options used with Binary compare -method.
    + * This class has some Binary Compare specifics in addition to general compare
    + * options.
    + */
    +class BinaryCompareOptions : public CCompareOptions
    I don't see any specifics? Just CCompareOptions with new name.

     
  • Kimmo Varis

    Kimmo Varis - 2009-06-24

    So we found another must be fixed issue - ByteCompare cannot use diffutils file_data struct. So we definitely need more refactoring before this patch can go in.

     
  • Kimmo Varis

    Kimmo Varis - 2009-06-24

    Also I'm really not sure about your base class for compare. It is base class for only couple of compare methods. What we do with classes that can't derive from your class? Have own base class for them? Or perhaps we need one base class just having filenames and Compare -function (CompareEngine). And then create class structure on that. It could be ContentCompare class deriving from CompareEngine. Then quick compare, byte compare and diffutils compare deriving from ContentCompare. Something like that.

    And it would be another patch first creating the class structure and refactoring existing compare engines to use them. Then we can add this new engine using those classes. So we don't need to first add new engine and then refactor it in next patch.

     
  • Matthias

    Matthias - 2009-06-24

    removed unused parts

     
  • Matthias

    Matthias - 2009-06-24

    removed DIFF.H
    modified Filelocation to hold the needed infos for compare.

    Createing ContentCompare makes not much sence, as a lot of work is to be done in foldercompare.
    The basic class should hold all common functios and flags,
    even they are not used in some classes.

    I create a new patch with modified FileLocation. With this content alL other method except full content (or some more refactoring) , will work perfect.

     
  • Kimmo Varis

    Kimmo Varis - 2009-06-24

    Better.

    > Createing ContentCompare makes not much sence, as a lot of work
    > is to be done in foldercompare
    > The basic class should hold all common functios and flags,
    > even they are not used in some classes.

    No. We don't want to have content compare functions for time/size compare engine.

    We clearly have two different kind of compare engines - comparing content and comparing file attributes. These both clearly need their own base classes in good class design.

    And attribute comparing will be very important in future, it is very fast and gives results lots of users need without wasting time with actual content. Lots of people only synchronize files with WinMerge and WinMerge is not good at that at the moment.

     
  • Matthias

    Matthias - 2009-06-24

    oh, we mist us.
    I mean foldercompare takes allready the decision what to do, so creating a new class for that makes not much sence.

     
  • Kimmo Varis

    Kimmo Varis - 2009-06-24

    I'm not meaning new class to replace folder compare. I'm talking about base class for compare engine classes.

     
  • Matthias

    Matthias - 2009-06-24

    So FileCompare can be our start. Let it grow.

     
  • Matthias

    Matthias - 2009-06-26

    useing filedescription in serperate array

     
  • Matthias

    Matthias - 2009-06-26

    to remove m_diffFileData takes a lot more todo.
    So now i'm passing only file.desc to BinaryCompare.

     
  • Kimmo Varis

    Kimmo Varis - 2009-08-14

    > So FileCompare can be our start. Let it grow.
    I don't understand this? Start of what? Grow? I don't want classes to grow, they are too fat already.

     
  • Matthias

    Matthias - 2009-08-15

    your own words
    >We clearly have two different kind of compare engines - comparing content
    >and comparing file attributes. These both clearly need their own base
    >classes in good class design.
    nothing else:=)

    But will take time to finish 3-4 month, sorry I'm out for some weeks.

     
  • Matthias

    Matthias - 2009-10-30

    removed unused header

     
  • Matthias

    Matthias - 2009-10-30

    what else should be done here?

     
  • Matthias

    Matthias - 2009-12-22

    new basic class for foldercompare

     
  • Matthias

    Matthias - 2009-12-22

    please have a look to that patch.
    Filecopmare will be a basic class
    Binary the new binary compare.

     
  • Kimmo Varis

    Kimmo Varis - 2009-12-22

    The patch adds new code that is not called? Probably copies code from other classes?

    I know this is copied code in BinaryCompare.cpp:
    + * @brief Compare two specified files, byte-by-byte
    + * @param [in] piAbortable Interface allowing to abort compare
    + * @return allways true

    +/**
    + * @brief Default destructor.
    + */
    +BinaryCompare::~BinaryCompare()
    +{
    +}
    No need for empty destructor.

    but I'd add comment saying it returns always true because byte-by-byte compare cannot fail.

    In BinaryCompare.h:
    +class CompareOptions;
    +class CommonCompareOptions;
    +class BinaryCompareOptions;
    +class IAbortable;
    +struct FileLocation;
    +struct file_data;

    Are all these forward declarations really needed?

    In FileCompare.cpp:

    +/**
    + * @brief Default destructor.
    + */
    +FileCompare::~FileCompare()
    +{
    +}

    No need for empty destructor.

    In FileCompare.h:
    + bool SetFilename(FileLocation *location, int fdesc[2]);
    + bool SetFilename(FileLocation *location, int fdesc[2], bool lLimited);
    Why have return value if they always return true? Use b prefix for bool arguments. What does that limiting mean?

    + bool m_lLimited; //info compare of limited size
    Use b as a prefix for booleans.

    If you intent FileCompare class to not be used but only inherited then you should make it abstract class.

     
  • Matthias

    Matthias - 2009-12-22

    I'd done most changes.
    for comparefiles I keeped a return value for later use.
    boolblLimited means:
    I case of method full content and filesize is over max we switch over to binary.
    If now the result is diff, I want to give a message back to GUI that the compare is done in a diffrent method.

     
1 2 > >> (Page 1 of 2)

Log in to post a comment.