Refactor to localize/minimize CPP #529

bergmark · 2017-03-21T19:50:42Z

Ideally I'd have one module defining all the CPP, it makes it easier to re-use and removes a lot of noise. This can be done incrementally by moving any of it to a new Data.Aeson.Compat module.

We probably can't extract everything, but here's the current list of potential suspects:

Data/Aeson/Encoding/Builder.hs:#if MIN_VERSION_bytestring(0,10,4)
Data/Aeson/Encoding/Builder.hs:#if !(MIN_VERSION_bytestring(0,10,4))
Data/Aeson/Internal/Time.hs:#if MIN_VERSION_base(4,7,0)
Data/Aeson/Parser/Internal.hs:#if MIN_VERSION_ghc_prim(0,3,1)
Data/Aeson/Parser/Internal.hs:#if MIN_VERSION_ghc_prim(0,3,1)
Data/Aeson/Parser/Internal.hs:#if MIN_VERSION_ghc_prim(0,3,1)
Data/Aeson/Parser/Unescape.hs:#ifdef CFFI
Data/Aeson/TH.hs:#if __GLASGOW_HASKELL__ >= 800
Data/Aeson/TH.hs:#if MIN_VERSION_template_haskell(2,8,0) && !MIN_VERSION_template_haskell(2,10,0)
Data/Aeson/TH.hs:#if MIN_VERSION_template_haskell(2,8,0)
Data/Aeson/TH.hs:#if MIN_VERSION_template_haskell(2,7,0) && !(MIN_VERSION_template_haskell(2,8,0))
Data/Aeson/TH.hs:#if MIN_VERSION_template_haskell(2,8,0) && !(MIN_VERSION_template_haskell(2,10,0))
Data/Aeson/TH.hs:#if MIN_VERSION_template_haskell(2,11,0)
Data/Aeson/TH.hs:#if MIN_VERSION_template_haskell(2,11,0)
Data/Aeson/TH.hs:#if MIN_VERSION_template_haskell(2,11,0)
Data/Aeson/TH.hs:#if MIN_VERSION_template_haskell(2,11,0)
Data/Aeson/TH.hs:#if MIN_VERSION_template_haskell(2,11,0)
Data/Aeson/TH.hs:#if MIN_VERSION_template_haskell(2,11,0)
Data/Aeson/TH.hs:#if MIN_VERSION_template_haskell(2,11,0)
Data/Aeson/TH.hs:#if MIN_VERSION_template_haskell(2,11,0)
Data/Aeson/TH.hs:#if MIN_VERSION_template_haskell(2,11,0)
Data/Aeson/TH.hs:#if !(MIN_VERSION_template_haskell(2,8,0)) || MIN_VERSION_template_haskell(2,10,0)
Data/Aeson/TH.hs:#if !(MIN_VERSION_template_haskell(2,11,0))
Data/Aeson/TH.hs:#if MIN_VERSION_template_haskell(2,8,0)
Data/Aeson/TH.hs:#if MIN_VERSION_template_haskell(2,8,0)
Data/Aeson/TH.hs:#if MIN_VERSION_template_haskell(2,8,0)
Data/Aeson/TH.hs:#if MIN_VERSION_template_haskell(2,8,0)
Data/Aeson/TH.hs:#if MIN_VERSION_template_haskell(2,8,0)
Data/Aeson/TH.hs:#if MIN_VERSION_template_haskell(2,11,0)
Data/Aeson/TH.hs:#if MIN_VERSION_template_haskell(2,11,0)
Data/Aeson/TH.hs:#if MIN_VERSION_template_haskell(2,9,0)
Data/Aeson/TH.hs:#if MIN_VERSION_template_haskell(2,8,0)
Data/Aeson/TH.hs:#if MIN_VERSION_template_haskell(2,10,0)
Data/Aeson/TH.hs:#if MIN_VERSION_template_haskell(2,8,0)
Data/Aeson/TH.hs:#if MIN_VERSION_template_haskell(2,8,0)
Data/Aeson/TH.hs:#if MIN_VERSION_template_haskell(2,10,0)
Data/Aeson/TH.hs:#if MIN_VERSION_template_haskell(2,8,0)
Data/Aeson/TH.hs:#if MIN_VERSION_template_haskell(2,8,0)
Data/Aeson/TH.hs:#if MIN_VERSION_template_haskell(2,8,0)
Data/Aeson/TH.hs:#if MIN_VERSION_template_haskell(2,8,0)
Data/Aeson/Types/FromJSON.hs:#if __GLASGOW_HASKELL__ >= 706
Data/Aeson/Types/FromJSON.hs:#ifndef HAS_COERCIBLE
Data/Aeson/Types/FromJSON.hs:#if HAS_COERCIBLE
Data/Aeson/Types/FromJSON.hs:#if HAS_COERCIBLE
Data/Aeson/Types/FromJSON.hs:#if HAS_COERCIBLE
Data/Aeson/Types/FromJSON.hs:#if HAS_COERCIBLE
Data/Aeson/Types/FromJSON.hs:#if HAS_COERCIBLE
Data/Aeson/Types/FromJSON.hs:#if HAS_COERCIBLE
Data/Aeson/Types/Generic.hs:#if MIN_VERSION_base(4,9,0)
Data/Aeson/Types/Internal.hs:#if __GLASGOW_HASKELL__ >= 800
Data/Aeson/Types/Internal.hs:#if !MIN_VERSION_unordered_containers(0,2,6)
Data/Aeson/Types/Internal.hs:#if MIN_VERSION_unordered_containers(0,2,6)
Data/Aeson/Types/ToJSON.hs:#if __GLASGOW_HASKELL__ >= 706
Data/Aeson/Types/ToJSON.hs:#if !(MIN_VERSION_bytestring(0,10,0))
Data/Aeson/Types/ToJSON.hs:#if MIN_VERSION_bytestring(0,10,0)
tests/Encoders.hs:#if __GLASGOW_HASKELL__ >= 706
tests/Encoders.hs:#if __GLASGOW_HASKELL__ >= 706
tests/Encoders.hs:#if __GLASGOW_HASKELL__ >= 706
tests/Encoders.hs:#if __GLASGOW_HASKELL__ >= 710
tests/Instances.hs:#if !MIN_VERSION_QuickCheck(2,9,0)
tests/Instances.hs:#if MIN_VERSION_base(4,7,0)
tests/Instances.hs:#if !MIN_VERSION_base(4,8,0) && !MIN_VERSION_QuickCheck(2,8,3)
tests/Instances.hs:#if !MIN_VERSION_base(4,8,0) && !MIN_VERSION_QuickCheck(2,8,3)
tests/Instances.hs:#if !MIN_VERSION_QuickCheck(2,9,0)
tests/Instances.hs:#if !MIN_VERSION_base(4,8,0)
tests/Properties.hs:#if MIN_VERSION_base(4,7,0)
tests/Properties.hs:#if __GLASGOW_HASKELL__ >= 706
tests/Properties.hs:#if __GLASGOW_HASKELL__ >= 706
tests/Properties.hs:#if __GLASGOW_HASKELL__ >= 706
tests/Properties.hs:#if __GLASGOW_HASKELL__ >= 706
tests/SerializationFormatSpec.hs:#if __GLASGOW_HASKELL__ >= 708
tests/SerializationFormatSpec.hs:#if __GLASGOW_HASKELL__ >= 708
tests/Types.hs:#if __GLASGOW_HASKELL__ >= 706
tests/UnitTests.hs:#if __GLASGOW_HASKELL__ >= 710
tests/UnitTests.hs:#if __GLASGOW_HASKELL__ >= 710

The text was updated successfully, but these errors were encountered:

phadej · 2017-03-21T19:55:01Z

at least template-haskell stuff could stay in .TH module, as it's local to that module.

bergmark · 2017-03-21T19:56:17Z

Sounds good, TH CPP can be pretty hard to pull out IME.

lorenzo · 2017-07-14T08:57:50Z

@phadej If I can get some guidance on how to approach this, I'd like to also tackle this

phadej · 2017-07-14T09:29:13Z

@lorenzo what are the files which are problematic for hindent, let's start with them. I'm also on FreeNode, feel free to /msg me.

lorenzo · 2017-07-14T09:40:36Z

The list is not too large:

hindent: ./Data/Aeson/Encode.hs: 11: 40: Parse error: Use
hindent: ./Data/Aeson/Encoding/Internal.hs: 231: 12: Parse error: >*<
hindent: ./Data/Aeson/Parser/Internal.hs: 223: 1: Parse error: data
hindent: ./Data/Aeson/Text.hs: 64: 3: Parse error: virtual }
hindent: ./Data/Aeson/TH.hs: 89: 1: Parse error: module
hindent: ./Data/Aeson/Types/FromJSON.hs: 25: 1: Parse error: {-# OPTIONS:GHC ...
hindent: ./Data/Aeson/Types/Generic.hs: 26: 1: Parse error: module
hindent: ./Data/Aeson/Types/Internal.hs: 147: 16: Parse error: >>=
hindent: ./Data/Aeson/Types/ToJSON.hs: 25: 1: Parse error: {-# OPTIONS:GHC ...
hindent: ./tests/Instances.hs: 225: 17: Parse error: :|
hindent: ./tests/Properties.hs: 313: 65: Parse error: EOF
hindent: ./tests/SerializationFormatSpec.hs: 73: 65: Parse error: EOF
hindent: ./tests/UnitTests.hs: 299: 84: Parse error: EOF

There are only 3 files there that are not related to this issue (those there the error reason shows an operator name)

phadej · 2017-07-14T09:58:37Z

seems to fail on the row after #include "overlapping-compat.h"

lorenzo · 2017-07-14T10:00:37Z

That's correct, after removing that it fails at another CPP tag.

phadej · 2017-07-14T10:11:29Z

When removing the includes in FromJSON, we run into hindent bug:

mihaimaruseac/hindent#383

data CoerceText a where
#if HAS_COERCIBLE
    CoerceText :: Coercible Text a => CoerceText a
#else
    CoerceText :: CoerceText a
#endif

In that case we could do (EDIT: hopefully it doesn't obfuscate the use-sites too much!)

#if HAS_COERCIBLE
#define CoercibleConstraint(a,b) Coercible a b =>
#else
#define CoercibleConstraint(a,b)
#endif

data CoerceText a where
    CoerceText :: CoercibleConstraint(Text,a) CoerceText a

That seems to work:

[FL973] ~/mess % cat aesoncoercible.hs| hindent --indent-size 4                       
#if HAS_COERCIBLE
#define CoercibleConstraint(a,b) Coercible a b =>
#else
#define CoercibleConstraint(a,b)
#endif
data CoerceText a where
    CoerceText :: CoercibleConstraint (Text, a) CoerceText a

% cat aesoncoercible.hs| hindent --indent-size 4 | cpp                 
# 1 "<stdin>"
# 1 "<built-in>"
# 1 "<command-line>"
# 1 "/usr/include/stdc-predef.h" 1 3 4
# 1 "<command-line>" 2
# 1 "<stdin>"





data CoerceText a where
    CoerceText :: CoerceText a

% cat aesoncoercible.hs| hindent --indent-size 4 | cpp -D HAS_COERCIBLE
# 1 "<stdin>"
# 1 "<built-in>"
# 1 "<command-line>"
# 1 "/usr/include/stdc-predef.h" 1 3 4
# 1 "<command-line>" 2
# 1 "<stdin>"





data CoerceText a where
    CoerceText :: Coercible Text a => CoerceText a

In other words, the trick is to make #defines to make rest of the code look like Haskell-enough :)

phadej · 2017-07-14T10:15:11Z

For #include, I opened an issue: mihaimaruseac/hindent#430

It might be a blocker to proceed with hindent task in aeson. You could do other cleanups though (like above), if @bergmark agrees it's what he had in mind?

phadej · 2017-07-14T12:06:28Z

@lorenzo heads-up: if you install hindent from my branch: mihaimaruseac/hindent#431 then it should handle #include, and you may proceed.

lorenzo · 2017-07-14T13:37:06Z

@phadej is there a decision on changing the CPP declarations the way you showed earlier?

phadej · 2017-07-14T15:00:21Z

@lorenzo it's up to @bergmark, I have no authority here :(

bergmark · 2017-07-14T16:47:08Z

How about this? Would it cause more duplication elsewhere?

#if HAS_COERCIBLE
data CoerceText a where
    CoerceText :: Coercible Text a => CoerceText a
#else
data CoerceText a where
    CoerceText :: CoerceText a
#endif

phadej · 2017-07-14T16:58:56Z

That constraint is repeated multiple times in the file.

bergmark · 2017-07-14T17:13:19Z

Hmm, am I missing something? But if that's the case then your suggestion sounds good!

phadej · 2017-07-14T17:26:12Z

% grep -B 1 -A 3 'Coercible ' Data/Aeson/Types/FromJSON.hs
import Data.Coerce (Coercible, coerce)
coerce' :: Coercible a b => a -> b
coerce' = coerce
#else
coerce' :: a -> b
--
#if HAS_COERCIBLE
    CoerceText :: Coercible Text a => CoerceText a
#else
    CoerceText :: CoerceText a
#endif
--
#if HAS_COERCIBLE
    Coercible Text a =>
#endif
    FromJSONKeyFunction a
fromJSONKeyCoerce = FromJSONKeyCoerce CoerceText
--
#if HAS_COERCIBLE
    Coercible a b =>
#endif
    FromJSONKeyFunction a -> FromJSONKeyFunction b
#if HAS_COERCIBLE

well, I guess, all of these can be repeated. IMHO CoercibleConstraint is more "semantical",but I remembered wrong, Coercible isn't used as much.

bergmark added help wanted compat labels Mar 24, 2017

bergmark mentioned this issue Jun 13, 2017

Use the th-abstraction library to factor out some TH boilerplate #553

Merged

phadej mentioned this issue Jul 13, 2017

Use auto-formatting (hindent?) #527

Open

bergmark added the good first issue label Jun 14, 2020

bergmark mentioned this issue Jun 22, 2020

Compile times and memory usage - remove O2 optimization option? #790

Closed

bergmark removed good first issue help wanted labels Jun 22, 2020

bergmark added this to the next milestone Jun 22, 2020

bergmark linked a pull request Jun 22, 2020 that will close this issue

Add remove some dead CPP #791

Merged

bergmark closed this as completed in #791 Jun 27, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor to localize/minimize CPP #529

Refactor to localize/minimize CPP #529

bergmark commented Mar 21, 2017

phadej commented Mar 21, 2017

bergmark commented Mar 21, 2017

lorenzo commented Jul 14, 2017

phadej commented Jul 14, 2017

lorenzo commented Jul 14, 2017 •

edited

phadej commented Jul 14, 2017

lorenzo commented Jul 14, 2017

phadej commented Jul 14, 2017 •

edited

phadej commented Jul 14, 2017

phadej commented Jul 14, 2017

lorenzo commented Jul 14, 2017

phadej commented Jul 14, 2017

bergmark commented Jul 14, 2017

phadej commented Jul 14, 2017

bergmark commented Jul 14, 2017

phadej commented Jul 14, 2017

Refactor to localize/minimize CPP #529

Refactor to localize/minimize CPP #529

Comments

bergmark commented Mar 21, 2017

phadej commented Mar 21, 2017

bergmark commented Mar 21, 2017

lorenzo commented Jul 14, 2017

phadej commented Jul 14, 2017

lorenzo commented Jul 14, 2017 • edited

phadej commented Jul 14, 2017

lorenzo commented Jul 14, 2017

phadej commented Jul 14, 2017 • edited

phadej commented Jul 14, 2017

phadej commented Jul 14, 2017

lorenzo commented Jul 14, 2017

phadej commented Jul 14, 2017

bergmark commented Jul 14, 2017

phadej commented Jul 14, 2017

bergmark commented Jul 14, 2017

phadej commented Jul 14, 2017

lorenzo commented Jul 14, 2017 •

edited

phadej commented Jul 14, 2017 •

edited