1

I'm working on a homework assignment in which I'm required to use char arrays instead of strings and qsort/bsearch. In my call to bsearch below, I know I'm passing the wrong size of Entry, but I'm not sure how to get the real size, and my compareEntries function is therefore not finding the right objects.

Can anyone help me understand what I'm missing?

#include  <iostream>

using  namespace  std;


typedef Entry*  EntryPtr;
class  Dictionary
{
    public  :
        Dictionary( const char  *filename );
        ~Dictionary();
        const char  *lookupDefinition( const char  *word );

    private  :
        int m_numEntries;
        EntryPtr *m_entries;
        static int compareEntries(const void *leftSide, const void *rightSide);
};

int Dictionary::compareEntries(const void *leftSide, const void *rightSide)
{
    EntryPtr lside = (EntryPtr) leftSide;
    EntryPtr rside = (EntryPtr) rightSide;

    return strcmp(lside->Word, rside->Word);
}

const char *Dictionary::lookupDefinition(const char *word)
{
    if (m_numEntries == 0)
        return 0;

    EntryPtr result = (EntryPtr) bsearch(word, m_entries, 
        m_numEntries, sizeof(m_entries[0]), Dictionary::compareEntries);

    return result->Definition;
}

class Entry
{
        public:
                Entry(const char *line);
                char *Word;
                char *Definition;
};

int  main()
{
    Dictionary  dict( "C:\\path\\file.txt" );
    dict.lookupDefinition("chair");
    return  0;
}
alk
  • 69,737
  • 10
  • 105
  • 255
JMP
  • 7,734
  • 6
  • 33
  • 34
  • 2
    The size looks fine, I think the problem is that you're passing a `char*` as key, but your comparator expects two `Entry*` parameters. That ain't right. You need compareEntries to expect a `char*` and an `Entry*` (both actually passed as void*, of course), in that order. – Steve Jessop Mar 09 '10 at 22:16
  • 2
    @Poita_: because he's using `bsearch` as instructed. I guess the instructor wants to stick to "C with classes" at this stage, otherwise he'd be using `std::lower_bound`, or better yet a `std::map`, and be done by now ;-) – Steve Jessop Mar 09 '10 at 22:20
  • 1
    @Steve Jessop: Which I think is an absolutely horrible way to teach C++. Teach C, or teach C++ (or whatever else you want to teach). This is not legal C, and it's extremely bad C++. – David Thornley Mar 09 '10 at 22:25
  • You might also need to check the result of bsearch. – UncleBens Mar 09 '10 at 22:26
  • @David: You're probably right, but since I'm not volunteering to take over the remainder of JMP's C++ education, I will instead try to help out but tell him how it *could* be so much easier ;-) – Steve Jessop Mar 09 '10 at 22:29
  • @David : Yes, this seems like someone's C class that they never bothered to update for C++ -- @JMP pretend / understand you are learning C. – Hogan Mar 09 '10 at 22:32
  • OOPS. There's an error in my first comment above, which Mark Ransom's updated answer just made me realise. You should be expecting a `char*` and a `EntryPtr*`, not as I said a `char*` and a `Entry*`. From the docs: "The comparison function pointed to by compar shall be called with two arguments that point to the key object and to an array element, in that order." – Steve Jessop Mar 09 '10 at 22:36
  • @Steve That's void pointer for all of us. Btw I already pointed that out in my answer. – amit kumar Mar 09 '10 at 22:40

5 Answers5

2

Why doesn't sizeof(Entry) work?

Changed again -- I think the size should be the size of the pointer....

EntryPtr tmp = new Entry("");
tmp->Word = word;

EntryPtr result = (EntryPtr) bsearch(tmp, m_entries, 
        m_numEntries, sizeof(EntryPtr), Dictionary::compareEntries);
Hogan
  • 69,564
  • 10
  • 76
  • 117
  • Entry is not a pointer. EntryPtr is a typedef of a pointer type. This means you can use `sizeof(Entry)`. – amit kumar Mar 09 '10 at 22:17
  • @JMP : Did you try what I suggested in my answer? – Hogan Mar 09 '10 at 22:20
  • @Amit, he is using `sizeof(EntryPtr)` so it is still a pointer (since `m_entries` is an `Entry**`) I don't think this sizeof is his real problem though, see Mark Ransom's answer. – Adam W Mar 09 '10 at 22:20
  • 2
    m_entries is an array of pointers to Entry (a typedeffed pointer makes one's head spin, doesn't it?). The size of the item should therefore be the size of the pointer. – UncleBens Mar 09 '10 at 22:30
  • @Adam : His sizeof is wrong. It is the size of a pointer not an Entry. An entry is at least 3 pointers in size. See my solution posted above, uses both our points. – Hogan Mar 09 '10 at 22:31
  • @UncleBens : Sure arrays are strange... but this doc here (2nd example) suggests he needs the size of what is pointed to... http://www.cplusplus.com/reference/clibrary/cstdlib/bsearch/ – Hogan Mar 09 '10 at 22:38
  • @Hogan: Exactly, my comment says its the size of a pointer not an entry. `EntryPtr` is typecast to `Entry*` read carefully :) – Adam W Mar 09 '10 at 22:47
1

You do know that bsearch requires sorted input, right?

sizeof(m_entries[0]) looks perfectly fine to me.

Edit: Now I see the problem. Your Dictionary class contains an array of pointers. The problem is in the compareEntries function, where you cast passed pointers to EntryPtr; you need to cast them to EntryPtr * instead.

Edit 2: As pointed out by Amit Kumar, you also need to change the key parameter you send to bsearch, or you need to realize that the pointers you receive in compareEntries are not pointing to the same types and will need two different typecasts.

Community
  • 1
  • 1
Mark Ransom
  • 299,747
  • 42
  • 398
  • 622
1

The problem is that the comparator function used in bsearch expects word to be of type Entry* (or m_entries to be of type char**).

amit kumar
  • 20,438
  • 23
  • 90
  • 126
0

Read the manual carefully.

A summary of the points made by others, as well as a couple more issues:

  • Your usage of sizeof is correct.

  • You should pass a pointer to the Entry containing the key you want to look up. Actually the key can be anything, and it will be passed to the comparison function as the first argument, and you just need to cast both arguments to right types. (The comparison function should still correspond to the order the items were sorted by.)

  • The casts in the comparison function are not correct. The comparison function receives a pointer to the element (which in your case is a pointer to Entry, hence the comparison function receives pointers to pointers to Entry).

  • You cast the result to the wrong type. Again the function returns a pointer to an element in the array (pointer to pointer to Entry).

  • You don't check if the result is NULL, should the key not be there.

  • You could probably give up one level of indirection (do you really need an array of pointers instead of an array of Entries?)

  • You should take it as a good example of what people mean when they talk about the virtues of type-safety: in your code virtually all the types are mixed up and you are doing wrong things with wrong types, yet not a single complaint from the compiler. That's what you get if you mess with void*, unless you know exactly what you are doing.

For the fun of it, having an array of pointers it takes a ridiculous amount of indirection to get a result:

#include <cstdlib>
#include <string>
#include <iostream>

int compare_string(const void* a, const void* b)
{
    return ((const std::string*)a)->compare(**(const std::string**)b);
}

int main()
{
    std::string a("a"), b("b"), c("c");
    std::string* array[3] = { &a, &b, &c };
    std::string key = "b";
    std::string** result = (std::string**)bsearch(&key, array, 3, sizeof(std::string*), compare_string);
    if (result) std::cout << **result << '\n';
}

IMO, it would take less time to implement your own type-safe bsearch, than it takes to figure all this out and have it tested and debugged.

UncleBens
  • 40,819
  • 6
  • 57
  • 90
0

sizeof(Entry) would work. Mostly sizeof should be used on the type instead of an instance.

sizeof(Entry)

is preferable to

Entry e;
sizeof(e);

or

Entry* e;
sizeof(*e);

all give the same result.

#include "stdio.h"
class Entry {
  double e;
  int i;
};
int main() {
  Entry e;
  printf("%d\n", sizeof(e));
  printf("%d\n", sizeof(Entry));
  printf("%d\n", sizeof(*(&e)));
  return 0;
}
Hogan
  • 69,564
  • 10
  • 76
  • 117
JohnH
  • 1
  • 1
    It is preferable to apply sizeof to an instance, not a type name: if the type of the variable you are using is changed, `sizeof(var)` will still be correct, but `sizeof(TheTypeItUsedToBe)` necessarily won't. – UncleBens Mar 09 '10 at 23:58
  • Good point UncleBens. The original mistake was taking sizeof pointer (array item) and was a misunderstanding of pointer syntax, sizeof type would prevent, sizeof instance is probably better for the reasons UncleBens states. – JohnH Mar 10 '10 at 14:15
  • sizeof(Entry) is wrong, because the array is an array of pointers, not an array of Entrys. – Mark Ransom Mar 10 '10 at 18:28