Opened 10 years ago

Closed 10 years ago

Last modified 21 months ago

#4822 closed defect (fixed)

COMMON: Compile error - SWAP undeclared in algorithm.h

Reported by: salty-horse Owned by: lordhoto
Priority: normal Component: --Other--
Keywords: Cc:


While trying tom compile from SVN with clang, I encountered the following error:

In file included from engines/scumm/detection.cpp:26: In file included from ./base/plugins.h:31: In file included from ./common/util.h:30: In file included from ./common/str.h:29: In file included from ./common/array.h:29: ./common/algorithm.h:182:2: error: use of undeclared identifier 'SWAP'

algorithm.h uses SWAP without including util.h.

The reason the code compiles in gcc is that (after preprocessing) engines/scumm/detection.cpp includes common/util.h AFTER including common/algorithm.h. Since SWAP depends on the template arguments of sortPartition, gcc only instantiates it (and lookups its symbols) at the end of the translation unit.

See for details. There is an open issue about whether this is defined behavior with the C++ standards committee.

In any case, this is an ugly edge-case. However, it cannot easily be fixed by including common/util.h in common/algorithm.h. This causes a circular dependency between util.h -> str.h -> array.h -> algorithm.h -> util.h

A possible solution is to not use SWAP in algorithm.h.

Ticket imported from: #2971226. Ticket imported from: bugs/4822.

Change History (7)

comment:1 by fingolfin, 10 years ago

That str.h depends on array.h is an old hack, and should be fixed, by moving the definition of StringList to a better place. E.g. a new header file common/str-list.h. We might also want to rename it to StringArray at the same time. That would break the dependency cycle you mention.

However, we also should consider splitting up common/util.h, which contains a jumbled mix of things which accumulated there over the years. This goes beyond this bug report, of course, but here are some ideas for that anyway: * move the RandomSource stuff to common/random.h * get rid of the textconsole.h include (I only put it there to ease transition) * move the string tokenizer to its own file(s) * ...

comment:2 by fingolfin, 10 years ago

Summary: Compilation error - SWAP undeclared in algorithm.hCOMMON: Compile error - SWAP undeclared in algorithm.h

comment:3 by fingolfin, 10 years ago

I removed the array.h include from str.h. Can you please test again?

comment:4 by salty-horse, 10 years ago

After including common/util.h in common/algorithm.h, clang++ successfully compiles the code.

comment:5 by lordhoto, 10 years ago

Just added the common/util.h include in common/algorithm.h.

Anyway I'm all for splitting up util.h as Fingolfin suggested. We might even consider moving SWAP to algorithm.h (and along with it renaming it to "swap", since it's a function and not a macro). Sames goes for Common::MIN, Common::MAX btw.

I'm not sure whether we should move ABS and CLIP to algorithm.h too, but IMHO they should also be renamed to be all lowercase too.

comment:6 by fingolfin, 10 years ago

Owner: set to lordhoto
Resolution: fixed
Status: newclosed

comment:7 by digitall, 21 months ago

Component: --Other--
Note: See TracTickets for help on using tickets.