-1

I have written a small code which isolates my flawed approach/understanding in using macros to access or assign values to elements in a std::vector. Below is the snippet of the code.

#define mat(i,j,nrows) mat[((j)*(nrows))+(i)]


struct _STR1
{
 int nRows, nCols;
 std::vector < double >mat;
 std::vector < double >anothermat;
};

void Create_Data (int &nC, _STR1 * &_str)
{
 _str = new _STR1[nC];

 for (int myid = 0; myid < nC; myid++)
 {
  _str[myid].nRows = 100;
  _str[myid].nCols = 3;

  _str[myid].mat.resize (_str[myid].nRows * _str[myid].nCols);

  _str[myid].anothermat.resize (_str[myid].nRows * _str[myid].nCols);

  for (int i_row = 0; i_row < _str[myid].nRows; i_row++)
    {
      _str[myid].mat (i_row, 0, _str[myid].nRows) = 1.0e0;

      _str[myid].mat (i_row, 1, _str[myid].nRows) = 1.0e0;

      _str[myid].mat (i_row, 2, _str[myid].nRows) = 1.0e0;

      _str[myid].anothermat (i_row, 2, _str[myid].nRows) = 1.0e0;
    }
 }
}

If I comment "_str[myid].anothermat (i_row, 2, _str[myid].nRows) = 1.0e0;", I do not get any error. Otherwise I get the following error

error: call of an object of a class type without appropriate operator() or conversion functions to pointer-to-function type

I think my understanding in using a macro is wrong but I am unable to understand why this is so.

Can anyone please tell me why this approach is wrong and why I have the error in one case whereas the other does not. Is my usage of macro correct?

Suman Vajjala
  • 217
  • 1
  • 2
  • 8
  • 2
    Unrelated to your question and problem, but names beginning with leading underscore and followed by an upper-case letter (like for example `_STR1`) are *reserved* for the "implementation" (compiler and the standard library). Don't use such names in your own code. See e.g. [this QA for more details](https://stackoverflow.com/questions/228783/what-are-the-rules-about-using-an-underscore-in-a-c-identifier). – Some programmer dude Sep 12 '18 at 06:32
  • You're not using a macro. Are you sure that's the only line you get an error on, what about the line above it? – Tas Sep 12 '18 at 06:32
  • As for your error, `_str[myid].anothermat` is a *vector*. It doesn't have a function-call operator. What are you really trying to do there? What is the purpose of that line? Same with the lines using the vector `mat`? – Some programmer dude Sep 12 '18 at 06:33
  • Ooops I am sorry @Someprogrammerdude. I have added the macro now – Suman Vajjala Sep 12 '18 at 06:35
  • @Tas I added the macro now. – Suman Vajjala Sep 12 '18 at 06:35
  • Why are you naming your macro with the same name as the vector you are trying to access? You should not be doing that. – Remy Lebeau Sep 12 '18 at 06:35
  • With that macro and the code as you show it now, you should get a *lot* of more errors, since *all* uses of the symbol `mat` would be the macro. Even the declaration of the *member variable* `mat` inside your structure. There's a reason macros are usually all upper-case while variables and functions are not. – Some programmer dude Sep 12 '18 at 06:36
  • 1
    And to solve your problem, ***don't use macros!*** Create a simple (possibly templated) function which does what you want. – Some programmer dude Sep 12 '18 at 06:37
  • @Someprogrammerdude Yes. But with the following code, I do not get error if I comment out a single statement and the code runs. I understand that my logic is wrong? But why doesnt the compiler issue an error? – Suman Vajjala Sep 12 '18 at 06:38
  • If you only defined mat as a macro, why would you expect it to work for anothermat? That's why you are only getting an error on that line. In any case, you should give a different name to your macro - not the same as the member in the struct. And better yet, use functions instead of macros. – Gonen I Sep 12 '18 at 06:38
  • Will the code be correct if I comment this statement out- "_str[myid].anothermat (i_row, 2, _str[myid].nRows) = 1.0e0;" – Suman Vajjala Sep 12 '18 at 06:42
  • @Someprogrammerdude Thank you for your comments and advice. I am not an expert in programming, so I make these mistakes. I wanted to ask you if I comment this statement or line "_str[myid].anothermat (i_row, 2, _str[myid].nRows) = 1.0e0;" will my code be correct? Will it work the way it is supposed to work? – Suman Vajjala Sep 12 '18 at 06:49
  • @RemyLebeau What will happen if I name the macro with the same name as the vector? Is this bug? – Suman Vajjala Sep 12 '18 at 06:54
  • Your struct name _STR1 is undefined behaviour (https://en.cppreference.com/w/cpp/language/identifiers) as already mentioned by @someprogrammerdude. Personally I strongly dislike the _python++ style of names. – Paul Floyd Sep 12 '18 at 12:11

2 Answers2

1

Macros are considered bad practice.

Use a function instead. They are much less error prone (e.g. type safe).

You can use various techniques to have the same performance as with C macros, such as template functions.

In your case, I would simply define a wrapper class:

template <typename T>
class Mat {
  private:
    std::vecotr<T> mat;
    int nrows;
  public:
    Mat(const Matrix & mat, int nrows);
    const T& operator()(int i, int j) const {
        return mat[j * nrows + i];
    }
    T& operator()(int i, int j) {
        return mat[j * nrows + i];
    }
};
Jarod42
  • 203,559
  • 14
  • 181
  • 302
schorsch312
  • 5,553
  • 5
  • 28
  • 57
  • 1
    Your pseudo code is a bit too pseudo. Though the idea is sound. Also `getMat` can be `operator()`, so the OP won't need to change their code too much. – StoryTeller - Unslander Monica Sep 12 '18 at 06:44
  • Thank you. After I discovered the bug, this thought came to my mind. I am not a programming expert so I make these mistakes! Thank you very much – Suman Vajjala Sep 12 '18 at 06:45
  • I corrected the template placement and added the operator overload. – schorsch312 Sep 12 '18 at 06:53
  • Sorry to be a pain, but you also have incorrect matrix position calculation. And convention has matrices record the number of columns, not the number of rows. – Gonen I Sep 12 '18 at 07:58
0

Though your approach is unconventional, it's not technically wrong in the case of the first matrix. In the case of the second matrix you simply didn't write a macro for it, so anothermat (i_row, 2, _str[myid].nRows) doesn't get converted to anything, and since it's not valid C++ on its own your program can't be built.

schorsch312's answer shows a better approach (despite the typos); as it happens, I wrote something similar just yesterday.

Lightness Races in Orbit
  • 378,754
  • 76
  • 643
  • 1,055