Converting this code be turned into a macro

Question

Sample code I currently have:

a = b;

New logic I would like my macro to achieve:

if(b != "" and b != " ")
    a = b;

So I would like a macro like this if possible:

a = VALUE(b);

a and b are of type CString. I know I can achieve this by writing an inline method but I wondered if it is possible with a MACRO?

I have several variables where I want to apply this logic when assigning the value.

You cannot do that without an `else` clause; you need either `a = VALUE(a,b);` or just `VALUE(a,b);`. — Ken Y-N, Oct 13 '20 at 10:16
Preprocessor macros are the **last** option to consider. You would only ever implement one if there aren't any other feasible options. In this case a function (inline or otherwise) is perfectly suitable. — IInspectable, Oct 13 '20 at 11:54
@IInspectable Based on the various comments I have come up with an `inline` function and fleshed out an answer with added context. — Andrew Truckle, Oct 13 '20 at 14:06

Vlad Feinstein · Accepted Answer · 2020-10-14T00:02:40.023

1

A suggestion for the bulk-copy of CString sequential fields in your S_TALK_INFO structure:

The name of your SetValue function doesn't reflect what it's doing. I would replace it with:

bool IsValid(const CString& str)
{
  return !str.IsEmpty() && !_istspace(str[0]);
}

Is this kosher? You would have to maintain the names of the first and last fields, but you have to maintain more than that now...

void CWeekendMeetingDlg::SetWeekendMeetingInfo(S_TALK_INFO &rsTalkInfo)
{
  for (CString* pSrc = &rsTalkInfo.strAwayCong1, *pDst = &m_sWMInfo.strAwayCong1; 
    pSrc <= &rsTalkInfo.strClosingPrayer; 
    pDst++, pSrc++)
  {
    if(IsValid(*pSrc))
      *pDst = *pSrc;
  }
  // the "int" stuff
}

Re:UB - would that help (for each consecutive pair)?

static_assert(offsetof(S_TALK_INFO, strAwayCong2) - offsetof(S_TALK_INFO, strAwayCong1) 
    == sizeof(CString));

edited Oct 14 '20 at 00:02

answered Oct 13 '20 at 21:09

Vlad Feinstein

10,960
1
12
27

I was not aware we could iterate through the structure member variables like this. I will try your approach tomorrow. Thanks. – Andrew Truckle Oct 13 '20 at 22:18
1

@AndrewTruckle Technically this is relying on UB, though it will likely work with many implementations. See for example [Is it legal to index into a struct?](https://stackoverflow.com/questions/40590216/is-it-legal-to-index-into-a-struct) for more discussion and possible alternatives. – dxiv Oct 13 '20 at 23:32
@dxiv I would prefer not to rely on UB or “likely to work” concepts. – Andrew Truckle Oct 14 '20 at 04:34
@AndrewTruckle please read my last update about ‘static_assert’ - this ensures it will work – Vlad Feinstein Oct 14 '20 at 05:38
@AndrewTruckle My comment was only meant to advise of the UB, beyond that it's your call to make. As far as UBs go, this one has a long, even idiomatic, tradition going back to the C times. So, in a sense, it is "expected" to work, at least with the default alignment and padding. You'll find it used in public code, see for example Microsoft's [`XMMATRIX`](https://github.com/microsoft/DirectXMath/blob/29b8c1cc00b9ef49eb1b08d60e472d6ec9938ec4/Inc/DirectXMath.h#L490) where 16 consecutive floats are aliased to 4 vectors or a 2D array. – dxiv Oct 14 '20 at 05:49
1

@AndrewTruckle That said, if you want to stick to the letter of the standard, there is always the option to define an `operator []`, as shown in [one of the answers](https://stackoverflow.com/a/40590471/5538420) to the linked question, then write an index based `for` loop. – dxiv Oct 14 '20 at 05:49
1

@VladFeinstein The `static_assert` (when it passes) will increase the chance that it will all work, but it does not and can not guarantee it, because incrementing those pointers is still UB according to the letter of the standard. Not saying it does or will ever happen, but a compiler would be technically allowed to see that `pDst++` at the end of the first iteration is UB, and replace it with `abort()`, or ignore it altogether. – dxiv Oct 14 '20 at 05:58
2

@dxiv I opted to use the revised `IsValid` method and a `operator []` approach as per the linked answer. I use a `for` loop that iterates up to `22`. Working well. – Andrew Truckle Oct 14 '20 at 11:26
I agree with @dxiv - `[]` is a better way. Better yet would be to declare an `enum` with all the fields and use `std::array` that could be accesses both by those indexes and by generic iterators. Might be less work than implementing `operator[]`... – Vlad Feinstein Oct 14 '20 at 16:40
1

@VladFeinstein I can’t change the main structure variables. It works now so I won’t break it. – Andrew Truckle Oct 14 '20 at 17:37

score 0 · Answer 2 · answered Oct 13 '20 at 14:06

Based on the comments urging me towards using an inline function, and given my specific requirements, I have ended up doing the following.

This is my struct object whose definition can't be changed:

typedef struct tagPublicTalkInfo
{
    CString     strAwayCong1;
    CString     strAwayCong2;
    CString     strAwayBrother1;
    CString     strAwayBrother2;
    CString     strAwayTalkNumber1;      // AJT v20.1.9
    CString     strAwayTalkNumber2;      // AJT v20.1.9
    CString     strChairman;
    CString     strCong;
    CString     strHosp;
    CString     strReader;
    CString     strBrother;
    CString     strTheme;
    CString     strWTConductor;          // AJT v10.7.2
    CString     strInterpreter;          // AJT v16.2.1
    CString     strMisc;                 // AJT v16.2.1
    CString     strWatchtowerStudyTheme; // AJT v17.0.5
    CString     strServiceTalkTheme;     // AJT v17.0.7
    CString     strBibleVersesReader;    // AJT v20.0.1
    CString     strVideoHost;            // AJT v20.1.4
    CString     strVideoCohost;          // AJT v20.1.4
    CString     strOpeningPrayer;        // AJT v20.1.6
    CString     strClosingPrayer;        // AJT v20.1.6
    int         iSongStart;              // AJT v20.0.1
    int         iSongMiddle;             // AJT v20.0.1
    int         iSongEnd;                // AJT v20.0.1
    int         iThemeNumber;            // AJT v20.1.6

} S_TALK_INFO;

I added this new inline method:

void SetValue(CString& rStrCurrentValue, CString strNewValue)
{
    if (strNewValue != _T("") && strNewValue != _T(" "))
        rStrCurrentValue = strNewValue;
}

Finally, I adjusted my SetWeekendMeetingInfo method, so that it now looks like:

void CWeekendMeetingDlg::SetWeekendMeetingInfo(S_TALK_INFO &rsTalkInfo)
{
    //m_sWMInfo = rsTalkInfo;

    SetValue(m_sWMInfo.strAwayCong1, rsTalkInfo.strAwayCong1);
    SetValue(m_sWMInfo.strAwayCong2, rsTalkInfo.strAwayCong2);
    SetValue(m_sWMInfo.strAwayBrother1, rsTalkInfo.strAwayBrother1);
    SetValue(m_sWMInfo.strAwayBrother2, rsTalkInfo.strAwayBrother2);
    SetValue(m_sWMInfo.strAwayTalkNumber1, rsTalkInfo.strAwayTalkNumber1);
    SetValue(m_sWMInfo.strAwayTalkNumber2, rsTalkInfo.strAwayTalkNumber2);
    SetValue(m_sWMInfo.strChairman, rsTalkInfo.strChairman);
    SetValue(m_sWMInfo.strCong, rsTalkInfo.strCong);
    SetValue(m_sWMInfo.strHosp, rsTalkInfo.strHosp);
    SetValue(m_sWMInfo.strReader, rsTalkInfo.strReader);
    SetValue(m_sWMInfo.strBrother, rsTalkInfo.strBrother);
    SetValue(m_sWMInfo.strTheme, rsTalkInfo.strTheme);
    SetValue(m_sWMInfo.strWTConductor, rsTalkInfo.strWTConductor);
    SetValue(m_sWMInfo.strInterpreter, rsTalkInfo.strInterpreter);
    SetValue(m_sWMInfo.strMisc, rsTalkInfo.strMisc);
    SetValue(m_sWMInfo.strWatchtowerStudyTheme, rsTalkInfo.strWatchtowerStudyTheme);
    SetValue(m_sWMInfo.strServiceTalkTheme, rsTalkInfo.strServiceTalkTheme);
    SetValue(m_sWMInfo.strBibleVersesReader, rsTalkInfo.strBibleVersesReader);
    SetValue(m_sWMInfo.strVideoHost, rsTalkInfo.strVideoHost);
    SetValue(m_sWMInfo.strVideoCohost, rsTalkInfo.strVideoCohost);
    SetValue(m_sWMInfo.strOpeningPrayer, rsTalkInfo.strOpeningPrayer);
    SetValue(m_sWMInfo.strClosingPrayer, rsTalkInfo.strClosingPrayer);
    m_sWMInfo.iSongStart = rsTalkInfo.iSongStart;
    m_sWMInfo.iSongMiddle = rsTalkInfo.iSongMiddle;
    m_sWMInfo.iSongEnd = rsTalkInfo.iSongEnd;
    m_sWMInfo.iThemeNumber = rsTalkInfo.iThemeNumber;

}

It appears to work function correctly. As I say, I can't change the definition of my struct object but if you think this approach can be improved then please inform me.

(1) `void SetValue(CString& rStrCurrentValue, CString strNewValue)` should be `void SetValue(CString& rStrCurrentValue, const CString& strNewValue)`. (2) seeing two dozen identical assignments makes me think about `vector` or some other collection. (3) semantically, is `""` different in your struct from `" "`? Why do you allow both?Could there be another white-space symbol? (4) `strNewValue != _T("")` is clearer expressed by `!strNewValue.IsEmpty()` (5) is there an "empty" or "blank" value for your `int` members? — Vlad Feinstein, Oct 13 '20 at 17:27
@VladFeinstein (1). Why use `const &` for the second parameter? (2) Yes, I could build a collection with pointers to those `CString` variables but then I would need two collections and by the time I have coded that I will have more lines of code than what I have now I would think. (3) It is because the data is coming from my other software (from a database) where the default values are either empty or single space. (4) Yes, but that won't account for `" "` will it? So I used the same approach for both for consistency. (5) The `int` values are not used insource program so their defaults are OK. — Andrew Truckle, Oct 13 '20 at 19:43
For (1) - & is to avoid string copy, and const - to indicate (and enforce) that that CString won't be modified. (2) I'll post a code suggestion in an answer (too much code in it). (3) I would sanitize that data either in database (preferred) or in your data layer, right after you receive it. I mean - empty is empty, you don't want multiple values to indicate that. (4) will become moot after (3). (5) got it — Vlad Feinstein, Oct 13 '20 at 21:12

Converting this code be turned into a macro

2 Answers2