Opened 9 years ago

Closed 9 years ago

Last modified 10 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:
Game:

Description

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 http://gcc.gnu.org/bugzilla/show_bug.cgi?id=29131 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 Changed 9 years ago by fingolfin

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 Changed 9 years ago by fingolfin

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

comment:3 Changed 9 years ago by fingolfin

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

comment:4 Changed 9 years ago by salty-horse

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

comment:5 Changed 9 years ago by lordhoto

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 Changed 9 years ago by fingolfin

Owner: set to lordhoto
Resolution: fixed
Status: newclosed

comment:7 Changed 10 months ago by digitall

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