5

I have an array of (pointers to) arrays of different lengths, which I learned I could define using compound literals:

const uint8_t *const minutes[] = {
    (const uint8_t[]) {END},
    (const uint8_t[]) {1, 2, 3, 4, 5 END},
    (const uint8_t[]) {8, 9, END},
    (const uint8_t[]) {10, 11, 12, END},
    ...
}; 

gcc accepts this just fine, but clang says: pointer is initialized by a temporary array, which will be destroyed at the end of the full-expression. What does this mean? The code seems to be working, but then again, lots of things seem to work when they point to memory that's no longer allocated. Is this something I need to worry about? (Ultimately I only really need it to work with gcc.)

Update: Something fishy is going on. It says here that:

Compound literals yield lvalues. This means that you can take the address of a compound literal, which is the address of the unnamed object declared by the compound literal. As long as the compound literal does not have a const-qualified type, you can use the pointer to modify it.

   `struct POINT *p;
   p = &(struct POINT) {1, 1};

This example code seems to be doing exactly what I'm trying to do: a pointer to something defined by a compound literal. So is the clang error message legitimate? Will this end up pointing to unallocated memory when compiled with either clang or gcc?

Update 2: Found some documentation: "In C, a compound literal designates an unnamed object with static or automatic storage duration. In C++, a compound literal designates a temporary object, which only lives until the end of its full-expression." So it seems that clang is right to warn about this, and gcc probably ought to as well, but doesn't, even with -Wall -Wextra.

I can't guess why a useful C feature was removed from C++, and no elegant alternative way of accomplishing the same thing was provided.

Community
  • 1
  • 1
Josh
  • 2,039
  • 3
  • 20
  • 25
  • Are you compiling with `-W` flag when using gcc ? It might show up if you are not – GeoffreyB Jul 03 '15 at 17:56
  • Yep. -Wall -Wextra, which is apparently the same thing. – Josh Jul 03 '15 at 18:05
  • have you tried removing the unnecessary `(const uint8_t[])` at the beginning of each row? – Richard Hodges Jul 03 '15 at 18:11
  • 1
    @RichardHodges it's not unnecessary. That's the syntax for compound literals. Without it the compiler expects something that looks like a pointer, and instead sees something in curly braces and complains: `error: braces around scalar initializer for type 'const uint8_t* const` – Josh Jul 03 '15 at 18:20
  • In C, this is legal at file scope, not legal at block scope. Can you please edit your question to show the scope . In C++ compound literals are not allowed at all . – M.M Jul 05 '15 at 07:00
  • "I can't guess why a useful C feature was removed from C++, " - compound literals were added to C after C++ had already been standardized. I'd speculate that C++ does not now add compound literals because they are not necessary: there is universal initialization in C++11, and the use of C-style arrays is discouraged because they do not have value semantics. – M.M Jul 05 '15 at 07:02
  • So this isn't valid C++, huh? Bummer... :( – paulotorrens Nov 15 '16 at 08:06

3 Answers3

3

Well, clang is right, and this shall be done that way:

namespace elements
{
    const uint8_t row1[] = {END};
    const uint8_t row2[] = {1, 2, 3, 4, 5, END};
    ...
}
const uint8_t *const minutes[] = {
    elements::row1,
    elements::row2,
    ...
}; 

You can think of more C++ solution, like using std::tuple:

#include <tuple>

constexpr auto minutes = std::make_tuple(
    std::make_tuple(), 
    std::make_tuple(1,2,3,4,5),
    std::make_tuple(8,9,10)); 

#include <iostream>
#include <type_traits>
int main() {
    std::cout << std::tuple_size<decltype(minutes)>::value << std::endl;
    std::cout << std::tuple_size<std::remove_reference_t<decltype(std::get<1>(minutes))>>::value << std::endl;
}
PiotrNycz
  • 23,099
  • 7
  • 66
  • 112
  • Why I don't like your first option: it's messy. It's prone to typos, and If I re-order the rows or add a row later, there's lots of variable renaming that has to be done, or just leaving the row numbers out-of-order. Having said that, it does seem to be the most straightforward way to get the right in-memory representation of the data. – Josh Jul 07 '15 at 15:48
  • I'll have to look into your second option, with std::tuple and constexpr. I'm more comfortable with C than C++ (if you haven't guessed - but I'm using some C++ libraries for this project), and I'm coding for an embedded system with *tiny* memory, so I don't want to use any feature if I don't know exactly what's going on behind the curtain. (Yes, I realize the design philosophy behind C++, or OOP in general, is that you're not *supposed* to need to know the internal implementation, but... :) – Josh Jul 07 '15 at 15:54
  • @Josh If you afraid of wrong ordering - you can use `BOOST_PP_ENUM` - see http://www.boost.org/doc/libs/1_39_0/libs/preprocessor/doc/ref/enum.html. – PiotrNycz Jul 07 '15 at 17:43
  • 1
    @Josh When talking about future insertions, deletions - you can use old trick from [basic] times - use tens numbers initially (row10,row20, ...) Then you can easily add row15 at any time... – PiotrNycz Jul 07 '15 at 17:44
  • yeah, that's exactly the "ugly" I was talking about :) Anyway, In my case the data isn't going to change, and I don't even need the namespace because this data structure is defined in a separate file and the names of the inner parts won't have external linkage. So it's not going to be a maintenance nightmare... it's just inelegant. – Josh Jul 08 '15 at 03:57
1

Well that means, this expression

(const uint8_t[]) {1, 2, 3, 4, 5 END},

creates a temporary object — temporary because it doesn't have any name which can last beyond the expression of which it is a part — which gets destroyed at the end of the full expression, which means this:

 };

defines "the full expression", at which point all the temporary objects will get destroyed, and the array of pointers minutes holds pointers which point to destroyed objects, which is why the compiler is giving warning.

Hope that helps.

Nawaz
  • 353,942
  • 115
  • 666
  • 851
  • Yes, I suspected as much. So what do I *do* about it? :) – Josh Jul 03 '15 at 18:03
  • Define them as array first. Or better redesign it. Try use `std::array`. – Nawaz Jul 03 '15 at 18:06
  • Can you give an example of what you mean? There's got to be a way to do this that doesn't require creating dozens of individually-named variables that I don't actually want or intend to use... right? This compound literals method was one I got from an answer here on SO - is it actually wrong? – Josh Jul 03 '15 at 18:08
  • (This is for an embedded system, with 30k program storage and 2k ram, so I need the code to be as tiny, efficient, simple, and generally under-my-control as possible. I'm trying to avoid anything "fancy" like std::array.) – Josh Jul 03 '15 at 18:13
  • Can you define `(const uint8_t[]) {1, 2, 3, 4, 5 END}` as an array? Like `const uint8_t a0[] = {1, 2, 3, 4, 5 END};` and so on. And then use them as elements of `minutes`. – Nawaz Jul 03 '15 at 18:19
  • I'm trying to avoid creating dozens of variables a0, a1, a2, a3, etc. It just feels *wrong*. The other alternative is to declare the size of the outer array, and then do `minutes[0] = ...; minutes[1] = ...'`, etc. But that's almost as inelegant. Plus, it happens at runtime instead of compile time. Is there really no way to declare this fairly simple structure in a single statement? – Josh Jul 03 '15 at 18:25
  • I've updated my question to point to a source which seems to suggest my code should work. Any thoughts? – Josh Jul 04 '15 at 05:03
  • In C++ the code is illegal; in C the object either lasts until the end of the enclosing function (if there is one), or for the lifetime of the program otherwise. – M.M Jul 05 '15 at 07:02
1

update: thanks to deniss for pointing out the flaw in the original solution.

static constexpr uint8_t END = 0xff;

template<uint8_t...x>
const uint8_t* make()
{
    static const uint8_t _[] = { x..., END };
    return _;
}

const uint8_t* const array[] = {
    make<>(),
    make<1, 2, 3, 4, 5>(),
    make<8, 9>(),
    make<10, 11, 12>()
};
Richard Hodges
  • 68,278
  • 7
  • 90
  • 142
  • This compiles, although it makes the compiled app bigger, and makes it take a bit more memory - not ideal for an embedded system. Any idea why that is? I'm also not sure what it's doing - as I understand it a static variable should allocate space once that persists between calls to the function. So how is this function allocating new space each time it's called? – Josh Jul 03 '15 at 19:02
  • It's generating one static function per different row of numbers. Each static function will contain a static array (and a hidden mutex). When you say more memory, how much are we talking? After optimisation a few hundred bytes might be worth the trade off for ease of maintenance. – Richard Hodges Jul 03 '15 at 19:05
  • Note: for the same number of parameters, this function generates only 1 array (filled with values from the first invocation). –  Jul 05 '15 at 05:33
  • Yup that's true. That's an oversight of mine. – Richard Hodges Jul 05 '15 at 05:54
  • @MattMcNabb hah! yay! :-) I use `_` in these functors and lambdas like this because it seems reasonably easy to me to interpret it as 'it' or 'the relevant thing'. – Richard Hodges Jul 05 '15 at 07:36